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
