balazske added inline comments.
================
Comment at: clang/include/clang/AST/ASTImportError.h:17
+
+#include "clang/AST/APValue.h"
+#include "llvm/Support/Error.h"
----------------
This header is probably not needed.
================
Comment at: clang/include/clang/AST/ASTImportError.h:22
+
+class ImportError : public llvm::ErrorInfo<ImportError> {
+public:
----------------
balazske wrote:
> Rename to `ASTImportError`.
The rename to `ASTImportError` is better in a separate patch. The rename would
touch code in much more places and there is a rule to make independent changes
in separate patches. Still I think this class should not be called just
`ImportError` if it has an own header (it is not an internal-like class of
`ASTImporter` as is used to be).
================
Comment at: clang/include/clang/AST/ASTImporterLookupTable.h:17
+#include "clang/AST/ASTImportError.h"
#include "clang/AST/DeclBase.h" // lookup_result
----------------
phyBrackets wrote:
> balazske wrote:
> > This include is not needed here.
> Hey thanks for reviewing , I have a doubt if I remove this include it from
> here , then I need to include this header in the ASTImporterSharedState.h ,
> now the ASTImporterSharedState.h and ASTImporter.h both have ASTImportError.h
> included , isn't this affect on ASTImporter.cpp file on conflicting over
> ASTImportError ? Strangely it builds fine on my system tho .
> isn't this affect on ASTImporter.cpp file on conflicting over ASTImportError ?
This is no problem, this is why the include guard `#ifndef
LLVM_CLANG_AST_ASTIMPORTERROR_H` is there.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D124774/new/
https://reviews.llvm.org/D124774
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
