aaron.ballman added a comment. Recap: aside from the function merging behavior and the possible question about assertions in tests, I think this is ready to go.
================ Comment at: clang/lib/Sema/SemaDecl.cpp:27 #include "clang/AST/NonTrivialTypeVisitor.h" +#include "clang/AST/Randstruct.h" #include "clang/AST/StmtCXX.h" ---------------- void wrote: > aaron.ballman wrote: > > Is this include necessary? > Yes. There's a call to `randstruct::randomizeStructureLayout` below. Oh derp, I missed that. :-) ================ Comment at: clang/unittests/AST/RandstructTest.cpp:154-158 +#ifdef _WIN32 + const field_names Expected = {"lettuce", "bacon", "mayonnaise", "tomato"}; +#else + const field_names Expected = {"mayonnaise", "bacon", "tomato", "lettuce"}; +#endif ---------------- void wrote: > aaron.ballman wrote: > > Any idea what's gone wrong here? (Do we have a bug to file because these > > come out reversed? If so, can you add a FIXME comment here that we expect > > this test to change someday?) > I think it's just a case where Windows' algorithm for `std::mt19937` is > subtly different than the one for Linux. I'm not sure we should worry about > it too much, to be honest. As long as it produces a deterministic output on > the same platform we should be fine. I think it's expected that the same > compiler/environment is used during all compilation steps. (I.e., one's not > going to compile a module on Windows for a kernel build on Linux.) Okay, that's a great reason for this to be left alone. ================ Comment at: clang/unittests/AST/RandstructTest.cpp:208-213 +// FIXME: Clang trips an assertion in the DiagnosticsEngine when the warning is +// emitted while running under the test suite: +// clang/lib/Frontend/TextDiagnosticPrinter.cpp:150: virtual void +// clang::TextDiagnosticPrinter::HandleDiagnostic(clang::DiagnosticsEngine::Level, +// const clang::Diagnostic&): Assertion `TextDiag && "UnExpected diagnostic +// outside source file processing"' failed. ---------------- void wrote: > aaron.ballman wrote: > > Is the assertion unrelated to the changes in your patch and you just happen > > to hit it with this test? (I get worried when tests trigger assertions.) > To be honest, I haven't looked at these tests. I inherited them from the > previous code base. I'll revisit these and probably delete them if they're > not useful. Okay, that sounds good to me. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D121556/new/ https://reviews.llvm.org/D121556 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits