Page MenuHomePhabricator

[clang][ASTImporter] Simplify code of attribute import [NFC].
ClosedPublic

Authored by balazske on Sep 30 2021, 3:57 AM.

Details

Summary

The code of ASTImporter::Import(const Attr *) was repetitive,
it is now simplified. (There is still room for improvement but
probably only after big changes.)

Diff Detail

Event Timeline

balazske created this revision.Sep 30 2021, 3:57 AM
balazske requested review of this revision.Sep 30 2021, 3:57 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 30 2021, 3:57 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
martong accepted this revision.Sep 30 2021, 5:56 AM

Awesome! Thanks!

Please indicate in the title that this is an [NFC].

This revision is now accepted and ready to land.Sep 30 2021, 5:56 AM
balazske retitled this revision from [clang][ASTImporter] Simplify code of attribute import. to [clang][ASTImporter] Simplify code of attribute import [NFC]..Sep 30 2021, 6:28 AM
shafik added inline comments.Sep 30 2021, 9:37 AM
clang/lib/AST/ASTImporter.cpp
8516–8517

I am enthusiastic about reference members if we can somehow avoid them.

Can we use a struct to return both the Err and the pointer together? I don't like std::pair mainly b/c first and second are not meaningful.

balazske updated this revision to Diff 376441.Oct 1 2021, 1:06 AM

AttrImporter owns now the result of import (until it is read),
instead of referenced error and returned pointer.

balazske updated this revision to Diff 376443.Oct 1 2021, 1:13 AM
balazske marked an inline comment as done.

Update the code comments.

steakhal added inline comments.
clang/lib/AST/ASTImporter.cpp
8585–8586

If it can be used only once, we should mark this function as an r-value function.
There is a similar macro already defined as LLVM_LVALUE_FUNCTION.

Later, when you would actually call this function, you need to std::move() the object, signifying that the original object gets destroyed in the process.

@aaron.ballman Do you think we need to define LLVM_RVALUE_FUNCTION or we can simply use the && in the function declaration?
I've seen that you tried to substitute all LLVM_LVALUE_FUNCTION macros in the past. What's the status on this?

aaron.ballman added inline comments.Oct 1 2021, 4:28 AM
clang/lib/AST/ASTImporter.cpp
8585–8586

The expected pattern is:

#if LLVM_HAS_RVALUE_REFERENCE_THIS
  void func() && {
  }
#endif

https://github.com/llvm/llvm-project/blob/main/clang/include/clang/ASTMatchers/ASTMatchersInternal.h#L609 (and elsewhere). However, I note that there are some unprotected uses (such as https://github.com/llvm/llvm-project/blob/main/clang/lib/Tooling/Syntax/BuildTree.cpp#L437) and so it may be a sign that we're fine to remove LLVM_HAS_RVALUE_REFERENCE_THIS because all our supported compilers do the right thing?

steakhal added inline comments.Oct 1 2021, 4:41 AM
clang/lib/AST/ASTImporter.cpp
8585–8586

You tried to eliminate that on Jan 22, 2020 with rGdfe9f130e07c929d21f8122272077495de531a38.
But according to git blame, the BuildTree.cpp#L437 was committed on Jul 9, 2019 with rG9b3f38f99085e2bbf9db01bb00d4c6d837f0fc00.
I'm confused.

aaron.ballman added inline comments.Oct 1 2021, 9:09 AM
clang/lib/AST/ASTImporter.cpp
8585–8586

I'm confused.

As am I. I went back and looked at the history here, and as best I can tell, I tried to get rid of lvalue functions, we threw it at bots, and for reasons I didn't capture anywhere I can find, had to revert it. Sorry for the troubles with not tracking that information! :-(

However, in my simple testing on godbolt, I can't find a version of MSVC that has issues with lvalue or rvalue reference overloads. We claim to still support MSVC 2017 (perhaps it's time to bump that to something more recent now that MSVC has changed its release schedule), so maybe there's an older version that's still an issue, but I would expect us to have spotted that given there are unprotected uses of rvalue ref overloads.

My gut feeling is that we should explore getting rid of LLVM_LVALUE_FUNCTION and LLVM_HAS_RVALUE_REFERENCE_THIS again as it's been almost two years since the last try. If there's a problematic version of MSVC, we might want to consider dropping support for it unless it persists in newer MSVC versions.

balazske updated this revision to Diff 377143.Oct 5 2021, 3:35 AM

Adding && and new asserts to use AttrImporter only once.

steakhal accepted this revision.Oct 5 2021, 3:39 AM

I think it looks great.
Thank you for addressing this.

This revision was landed with ongoing or failed builds.Oct 7 2021, 3:53 AM
This revision was automatically updated to reflect the committed changes.