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

Fri Apr 8 23:48:31 PDT 2022

MaskRay 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));
----------------
void wrote:
> 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.
> 
I was a subscriber only vaguely aware of this patch and mostly absent in the past 2 weeks on trips (which meant I spent really little time on reading patches) :)

I just hope that every option added is useful. A thing that is not so necessarily can be delayed until it is actually needed.

Just noticed that there is test coverage gap that the cc1 options are completely untested. There are unit tests, but no lit test.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121556