[PATCH] D107292: [clang] adds warning to alert user when they use alternative tokens as references

[Arthur O'Dwyer via Phabricator via cfe-commits]

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