aaron.ballman 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: > 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. ================ Comment at: clang/unittests/AST/RandstructTest.cpp:41 + +std::unique_ptr<ASTUnit> makeAST(const std::string &SourceCode, + bool ExpectErr = false) { ---------------- void wrote: > MaskRay wrote: > > Use static. See > > https://llvm.org/docs/CodingStandards.html#anonymous-namespaces > Nah. FWIW, I agree with this feedback -- please follow the coding standards unless there's strong incentive not to (which I don't think there is here). Sorry for not catching this before during review. 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