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
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
