aaron.ballman accepted this revision.
aaron.ballman added a comment.
This revision is now accepted and ready to land.
LGTM!
================
Comment at: clang/lib/Basic/DiagnosticIDs.cpp:656
std::vector<std::string> DiagnosticIDs::getDiagnosticFlags() {
- std::vector<std::string> Res;
+ std::vector<std::string> Res{"-W", "-Wno-"};
for (size_t I = 1; DiagGroupNames[I] != '\0';) {
----------------
cjdb wrote:
> aaron.ballman wrote:
> > I presume this change doesn't have any negative effects in
> > `Driver::HandleAutocompletions()`? (That's currently the only consumer of
> > this API.)
> All tests pass, so I hope not?
Sure, we can go with that. I also spot-checked and I don't think this should
have a negative impact there.
================
Comment at: clang/lib/Driver/Driver.cpp:2010
+ if (C.getArgs().hasArg(options::OPT_print_diagnostic_options)) {
+ std::vector<std::string> Flags = DiagnosticIDs::getDiagnosticFlags();
+ for (std::size_t I = 0; I != Flags.size(); I += 2)
----------------
cjdb wrote:
> aaron.ballman wrote:
> > Declare the type as a const reference?
> >
> > (Btw, when uploading patches, you should give them plenty of patch context
> > -- that's what allows reviewers to suggest changes directly in Phab, but it
> > also gives more information to reviewers so they don't have to leave the
> > review to go to a code editor. FWIW, I usually use `-U999` to add context.)
> > Declare the type as a const reference?
>
> I'm not seeing the advantage of doing this here. Happy to make it const, but
> the reference doesn't add a benefit and potentially introduces confusion.
>
> > (Btw, when uploading patches, you should give them plenty of patch context
> > -- that's what allows reviewers to suggest changes directly in Phab, but it
> > also gives more information to reviewers so they don't have to leave the
> > review to go to a code editor. FWIW, I usually use -U999 to add context.)
>
> Sorry about that. I usually use `arc diff` to upload patches, but have been
> having problems with it recently and did a rushed manual upload instead.
> I'm not seeing the advantage of doing this here. Happy to make it const, but
> the reference doesn't add a benefit and potentially introduces confusion.
I still reflexively assume that some older compilers aren't as good at move
operations as they should be, and this would be inducing a copy of a pretty
large vector. Using a const reference means the result of the call is lifetime
extended and there's no chance of a big copy. However, I don't insist as I
think we should be able to rely on move operations these days?
> Sorry about that. I usually use arc diff to upload patches, but have been
> having problems with it recently and did a rushed manual upload instead.
Ah, no worries! I just mentioned it in case you weren't aware. :-)
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D126796/new/
https://reviews.llvm.org/D126796
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
