This is an archive of the discontinued LLVM Phabricator instance.

[clang][NFC][AST] rename the ImportError to ASTImportError
ClosedPublic

Authored by phyBrackets on May 10 2022, 2:57 PM.

Details

Summary

this patch is the continuation of my previous patch regarding the ImportError in ASTImportError.h

Diff Detail

Event Timeline

phyBrackets created this revision.May 10 2022, 2:57 PM
Herald added a reviewer: shafik. · View Herald Transcript
Herald added a project: Restricted Project. · View Herald Transcript
Herald added a subscriber: martong. · View Herald Transcript
phyBrackets requested review of this revision.May 10 2022, 2:57 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 10 2022, 2:57 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript

What are the benefits of this renaming? I mean is there a name clash? Do we have another kind of "import" in Clang or in some of the dependent projects, don't we?

What are the benefits of this renaming? I mean is there a name clash? Do we have another kind of "import" in Clang or in some of the dependent projects, don't we?

AS it suggested by @balazske in the previous patch , and I also think if we moved the ImportError into it's own header then it's better to be named as ASTImportError .

Initially the ImportError was very related to ASTImporter but now the class is used at other less related places and files. ImportError is a too generic name for this unless it can be used at any other "import" in clang which is not the case (the class can be extended with other special data about the import error). Maybe the whole ASTImporter should go into a new namespace, specially if ASTImporter.cpp would be split into parts?

I found one other place in LibASTImporter.rst where ImportError is used. LLDB should be checked too.

I found one other place in LibASTImporter.rst where ImportError is used. LLDB should be checked too.

Yeah , I update the file LibASTImporter.rst, I'm not sure if there is need to change the ImportError that are used in lldb ?

martong accepted this revision.May 12 2022, 12:20 AM

Yeah, okay, this patch makes sense now that I've seen a clash with python's ImportError . I've checked lldb c++ files and ImportError is not used. LGTM.

This revision is now accepted and ready to land.May 12 2022, 12:20 AM

LLDB should be at least compilable after the change. If it has an internal ImportError we must not change it. If it uses the renamed class we should rename it, or make an "using" alias and no rename (but not this is the best solution).