[PATCH] D93344: [flang] Handle multiple names for same operator

[Tim Keith via Phabricator via llvm-commits]

tskeith added a comment.

In D93344#2456531 <https://reviews.llvm.org/D93344#2456531>, @klausler wrote:

> Could the same effect be accomplished by instead canonicalizing `operator(...)` names in the output of the prescanner?

Yes, and that would simplify life in `resolve-names.cpp`. But it would mean that a user that consistently used `operator(==)`, for example, might get diagnostics that refer to `operator(.eq.)` (or vice versa, depending on how we canonicalized). As it is, we use one of the names that appears in the source.



================
Comment at: flang/lib/Semantics/resolve-names-utils.cpp:32
 
+static const std::string operatorPrefix{"operator("};
+
----------------
klausler wrote:
> This might be an instance of a static variable with a nontrivial constructor, which is typically verboten in LLVM.  Can you make it `constexpr` instead?
I tried that but with clang 9 I get:
`error: constexpr variable cannot have non-literal type 'const std::string'`

I can change it to `static constexpr const char *` and create the string when needed.



================
Comment at: flang/lib/Semantics/resolve-names-utils.cpp:167
 
+llvm::raw_ostream &operator<<(
+    llvm::raw_ostream &os, const GenericSpecInfo &info) {
----------------
klausler wrote:
> Is this code just for debugging during development?
Yes, like lots of other instances of `operator<<`.


================
Comment at: flang/lib/Semantics/resolve-names.cpp:505
   Symbol *FindInScope(const Scope &, const SourceName &);
+  template <typename T> Symbol *FindInScope(const T &name) {
+    return FindInScope(currScope(), name);
----------------
klausler wrote:
> This template seems to just call one of the two overloads declared beforehand, and may not be necessary.
It passes in the current scope so we can call `FindInScope` on a `parser::Name` or `SourceName` without passing in a Scope.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D93344/new/

https://reviews.llvm.org/D93344