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));
----------------
aaron.ballman wrote:
> MaskRay wrote:
> > 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.
> > I just hope that every option added is useful. A thing that is not so 
> > necessarily can be delayed until it is actually needed.
> 
> I think this option is useful. Windows' cmd.exe doesn't make it particularly 
> trivial to pipe contents to an argument in a command line, but also, IDEs 
> don't always make it obvious how you would pipe the seed content into a file 
> either. I don't see an issue with using `fstream` either; we use it when 
> necessary.
Ah, LG! 

Just need some tests for the two -f -cc1 options.


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

Reply via email to