Quuxplusone added a comment.
@dblaikie wrote:
> Not a huge objection - but minor quandry: What's the motivation for this
> patch?
AIUI, this diagnostic is a PR stunt, similar to Clang's existing
`-Wunicode-homoglyph`:
test.cpp:3:13: warning: treating Unicode character <U+037E> as identifier
character rather than as ';' symbol [-Wunicode-homoglyph]
return 0Íž
^
It's not that anyone would ever //intentionally// write code like `void
print(const std::string bitand s)`; it's that if someone copy-pastes such code
from Reddit into Clang, Clang will helpfully explain the trick to them. I'm
ambivalent as to whether this is worth it; but the patch is certainly tiny
enough that it's not obviously //not// worth it...
================
Comment at: clang/include/clang/Basic/DiagnosticParseKinds.td:1552
+ "use '%select{&|&&}0' when declaring %select{lvalue|rvalue and forwarding}1
references">,
+ InGroup<DiagGroup<"declare-references-with-symbols">>, DefaultWarn;
+
----------------
aaron.ballman wrote:
> Pretty sure you don't need `DefaultWarn` here (I think you'd use that for
> warnings that are off by default, like ones spelled with `Extension` instead
> of `Warning` or `ExtWarn`).
I suggest:
`"use '%select{&|&&}0' when declaring an %select{lvalue|rvalue or forwarding}0
reference"`
Besides the minor grammar tweak, this should avoid the need to pass the same
argument twice (as both argument 0 and argument 1). It's possible that this
ends up not working for some technical reason, but I'd be surprised if it
didn't Just Work.
================
Comment at: clang/lib/Parse/ParseDecl.cpp:5847-5849
+ constexpr int AmpAmp = 1;
+ Diag(Loc, diag::warn_declare_references_with_symbols)
+ << AmpAmp << AmpAmp;
----------------
aaron.ballman wrote:
> cjdb wrote:
> > aaron.ballman wrote:
> > > For consistency with the `Lvalue` case above.
> > I originally had `Rvalue`, but this is //technically// diagnosing both
> > rvalue refs and forwarding refs (hence the awkward name). I guess it
> > doesn't really matter at the parsing stage though, so this is just
> > commentary.
> Oh, good point on forwarding refs. Maybe change the previous one to `Amp` in
> that case?
In both cases, I'd write
```
Diag(Loc, diag::warn_declare_references_with_symbols)
<< (Kind == tok::ampamp);
```
or
```
<< static_cast<int>(Kind == tok::ampamp);
```
only if absolutely necessary (e.g. because the former doesn't build, or if it
runtime-errors).
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D107292/new/
https://reviews.llvm.org/D107292
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
