Page MenuHomePhabricator

[flang] Handle multiple names for same operator
ClosedPublic

Authored by tskeith on Dec 15 2020, 2:47 PM.

Details

Summary

Some operators have more than one name, e.g. operator(==), operator(.eq).
That was working correctly in generic definitions but they can also
appear in other contexts: USE statements and access statements, for
example.

This changes FindInScope to always look for each of the names for
a symbol. So an operator may be use-associated under one name but
declared private under another name and it will be the same symbol.
This replaces GenericSpecInfo::FindInScope which was only usable in
some cases.

Add a version of FindInScope() that looks in the current scope to
simplify many of the calls.

Diff Detail

Event Timeline

tskeith created this revision.Dec 15 2020, 2:47 PM
tskeith requested review of this revision.Dec 15 2020, 2:47 PM
Herald added a project: Restricted Project. · View Herald TranscriptDec 15 2020, 2:47 PM

Note: this change depends on D93343

I'm not sure how to layer one patch on top of another. I tried doing an "arc patch D93343" followed by "arc patch D93344". The second command said that it successfully committed the patch, but then it said "Cherry Pick Failed!" How do I combine both patches?

klausler accepted this revision.Dec 15 2020, 3:49 PM

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

flang/lib/Semantics/resolve-names-utils.cpp
32

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?

167

Is this code just for debugging during development?

flang/lib/Semantics/resolve-names.cpp
505

This template seems to just call one of the two overloads declared beforehand, and may not be necessary.

This revision is now accepted and ready to land.Dec 15 2020, 3:49 PM

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.

flang/lib/Semantics/resolve-names-utils.cpp
32

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.

167

Yes, like lots of other instances of operator<<.

flang/lib/Semantics/resolve-names.cpp
505

It passes in the current scope so we can call FindInScope on a parser::Name or SourceName without passing in a Scope.

I'm not sure how to layer one patch on top of another. I tried doing an "arc patch D93343" followed by "arc patch D93344". The second command said that it successfully committed the patch, but then it said "Cherry Pick Failed!" How do I combine both patches?

I don't know, but now D93343 has been submitted so you should be able to arc patch on top of the latest on main.

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.

Sure, but we would still be quoting and underlining the original source, so the user experience wouldn't be a total loss. Let me know if this is something that you'd like me to try to do.

tskeith updated this revision to Diff 312202.Dec 16 2020, 7:02 AM

Make operatorPrefix constexpr.

This revision was landed with ongoing or failed builds.Dec 16 2020, 7:07 AM
This revision was automatically updated to reflect the committed changes.