[PATCH] D121556: [randstruct] Add randomize structure layout support

[Bill Wendling via Phabricator via cfe-commits]

void added inline comments.


================
Comment at: clang/lib/Frontend/CompilerInvocation.cpp:4244
 
+  if (const Arg *A = Args.getLastArg(OPT_frandomize_layout_seed_file_EQ)) {
+    std::ifstream SeedFile(A->getValue(0));
----------------
MaskRay wrote:
> Why is -frandomize-layout-seed-file= needed? Can't the user use something like -frandomize-layout-seed=$(<file) ? Or backquotes for POSIX sh compatibility?
> 
> The impl uses the very uncommon header <fstream>.
That seems a bit clunky to me. If you don't like it, I can just remove the option entirely. Wish you would have mentioned these concerns earlier...like in the several weeks this has been in review.

The `fstream` header is used in other places. If there's a better alternative, please suggest one.



================
Comment at: clang/lib/Sema/SemaDecl.cpp:17852
+        !Record->isRandomized()) {
+      SmallVector<Decl *, 32> OrigFieldOrdering(Record->fields());
+      SmallVector<Decl *, 32> NewFieldOrdering;
----------------
MaskRay wrote:
> 32 may be too large. Consider the default inlined number of elements (fit the vector in 64 bytes).
Linux has many structures with far more fields. In fact, it's probably not useful to randomize structures with a small number of fields.


================
Comment at: clang/unittests/AST/RandstructTest.cpp:41
+
+std::unique_ptr<ASTUnit> makeAST(const std::string &SourceCode,
+                                 bool ExpectErr = false) {
----------------
MaskRay wrote:
> Use static. See https://llvm.org/docs/CodingStandards.html#anonymous-namespaces
Nah.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D121556/new/

https://reviews.llvm.org/D121556