Page MenuHomePhabricator

[NFC] Cleanup the overload of ASTImporter::import()
ClosedPublic

Authored by steakhal on Sep 4 2021, 4:35 AM.

Details

Summary

This patch aims to address the comment of a previous review:
https://reviews.llvm.org/D109237#inline-1040678

The original problem was the following:

`T` is substituted by `clang::Type`
Expected<T *> import(T *From) {
  auto ToOrErr = Importer.Import(From);
  //             ^^^^^^^^^^^^^^^^^^^^^
  if (!ToOrErr)
    return ToOrErr.takeError();
  return cast_or_null<T>(*ToOrErr);
  //     ^^^^^^^^^^^^^^^^^^^^^^^^^
}

Importer.Import() operates on const Type *, thus returns const Type *.
Later, at the return statement, we will try to construct an Expected<Type*>
from a const Type *, which failed with a miserable error message.

In all other cases importer.Import() results in a non-const version,
so everything works out just fine, but for clang::types, we should
really return a const version.

So, in case of T is a subclass of clang::Type, it will return a
Exprected<const T*> instead.

Diff Detail

Event Timeline

steakhal created this revision.Sep 4 2021, 4:35 AM
steakhal requested review of this revision.Sep 4 2021, 4:35 AM
steakhal updated this revision to Diff 370733.Sep 4 2021, 5:33 AM
steakhal edited the summary of this revision. (Show Details)

A minor style fixup.

martong accepted this revision.Sep 8 2021, 2:56 AM

Awesome! Thanks!

clang/lib/AST/ASTImporter.cpp
1170

Probably we should have a shorter type alias for Expected<const Type*> to be aligned with the rest of the existing type aliases we already have.
I am thinking about something like this:

using ExpectedQualType = llvm::Expected<QualType>;
using ExpectedType = llvm::Expected<const Type*>;
This revision is now accepted and ready to land.Sep 8 2021, 2:56 AM
steakhal added inline comments.Sep 8 2021, 3:18 AM
clang/lib/AST/ASTImporter.cpp
1170

I slightly disagree. Since this import aims to be used with the most specific derived type, we shouldn't expect too many uses of the raw Expected<const Type*> type to justify a type alias IMO.

For consistency though, I can see your point, but I'd like to hear what do the others think about this.
@shafik

balazske added inline comments.Sep 20 2021, 1:27 AM
clang/lib/AST/ASTImporter.cpp
1170

There is already ExpectedType, Expected<const Type *> could be ExpectedTypePtr.

shafik added inline comments.Sep 20 2021, 10:31 AM
clang/lib/AST/ASTImporter.cpp
1170

I feel that consistency aids readability and maintainability since it reduces surprises. If you see it done one way everywhere else then you wonder why is it different here or perhaps it creates confusion as to which way is the right way to do it for users contributing a patch.

steakhal updated this revision to Diff 375840.Sep 29 2021, 5:16 AM
steakhal marked 3 inline comments as done.

add type alias for ExpectedTypePtr

clang/lib/AST/ASTImporter.cpp
1170

Okay, fixed this.

martong accepted this revision.Sep 29 2021, 6:06 AM

Great, thanks!

This revision was landed with ongoing or failed builds.Sep 30 2021, 2:53 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptSep 30 2021, 2:53 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript