[PATCH] D124774: [clang][ASTImporter][NFC]: Move clang::ImportError into own header and rename it .
phyBrackets added inline comments. Comment at: clang/include/clang/AST/ASTImporterLookupTable.h:17 +#include "clang/AST/ASTImportError.h" #include "clang/AST/DeclBase.h" // lookup_result 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 . 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
[PATCH] D124774: [clang][ASTImporter][NFC]: Move clang::ImportError into own header and rename it .
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 {
+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
[PATCH] D124774: [clang][ASTImporter][NFC]: Move clang::ImportError into own header and rename it .
phyBrackets added inline comments.
Comment at: clang/include/clang/AST/ASTImportError.h:22
+
+class ImportError : public llvm::ErrorInfo {
+public:
balazske wrote:
> 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).
So, what do you suggest , for this patch should I go with name //ImportError//
only ? And rename it as //ASTImportError// in a separate patch ?
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
[PATCH] D124774: [clang][ASTImporter][NFC]: Move clang::ImportError into own header and rename it .
phyBrackets added inline comments. Comment at: clang/include/clang/AST/ASTImporterLookupTable.h:17 +#include "clang/AST/ASTImportError.h" #include "clang/AST/DeclBase.h" // lookup_result balazske wrote: > 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. > > ahh yes! Thanks 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
[PATCH] D124774: [clang][ASTImporter][NFC]: Move clang::ImportError into own header and rename it .
balazske added inline comments.
Comment at: clang/include/clang/AST/ASTImportError.h:1
+//===- ASTImporter.h - Import Error while Importing AST ---*- C++ -*-===//
+//
balazske wrote:
>
Comment at: clang/include/clang/AST/ASTImportError.h:22
+
+class ImportError : public llvm::ErrorInfo {
+public:
phyBrackets wrote:
> balazske wrote:
> > 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).
> So, what do you suggest , for this patch should I go with name
> //ImportError// only ? And rename it as //ASTImportError// in a separate
> patch ?
Yes: Do not change the class name here (but header should be called
ASTImportError.h). In a next patch make only a rename.
Comment at: clang/include/clang/AST/ASTImportError.h:52
+#endif // LLVM_CLANG_AST_ASTIMPORTERROR_H
\ No newline at end of file
Please add newline. I do not know if this can cause buildbot failures.
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
[PATCH] D124774: [clang][ASTImporter][NFC]: Move clang::ImportError into own header and rename it .
phyBrackets updated this revision to Diff 426992.
phyBrackets added a comment.
Address inline comments
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D124774/new/
https://reviews.llvm.org/D124774
Files:
clang/include/clang/AST/ASTImportError.h
clang/include/clang/AST/ASTImporter.h
clang/include/clang/AST/ASTImporterLookupTable.h
clang/include/clang/AST/ASTImporterSharedState.h
Index: clang/include/clang/AST/ASTImporterSharedState.h
===
--- clang/include/clang/AST/ASTImporterSharedState.h
+++ clang/include/clang/AST/ASTImporterSharedState.h
@@ -14,6 +14,7 @@
#ifndef LLVM_CLANG_AST_ASTIMPORTERSHAREDSTATE_H
#define LLVM_CLANG_AST_ASTIMPORTERSHAREDSTATE_H
+#include "clang/AST/ASTImportError.h"
#include "clang/AST/ASTImporterLookupTable.h"
#include "clang/AST/Decl.h"
#include "llvm/ADT/DenseMap.h"
Index: clang/include/clang/AST/ASTImporterLookupTable.h
===
--- clang/include/clang/AST/ASTImporterLookupTable.h
+++ clang/include/clang/AST/ASTImporterLookupTable.h
@@ -14,7 +14,6 @@
#ifndef LLVM_CLANG_AST_ASTIMPORTERLOOKUPTABLE_H
#define LLVM_CLANG_AST_ASTIMPORTERLOOKUPTABLE_H
-#include "clang/AST/ASTImportError.h"
#include "clang/AST/DeclBase.h" // lookup_result
#include "clang/AST/DeclarationName.h"
#include "llvm/ADT/DenseMap.h"
Index: clang/include/clang/AST/ASTImporter.h
===
--- clang/include/clang/AST/ASTImporter.h
+++ clang/include/clang/AST/ASTImporter.h
@@ -14,7 +14,7 @@
#ifndef LLVM_CLANG_AST_ASTIMPORTER_H
#define LLVM_CLANG_AST_ASTIMPORTER_H
-#include "clang/AST/ASTImporterLookupTable.h"
+#include "clang/AST/ASTImportError.h"
#include "clang/AST/DeclBase.h"
#include "clang/AST/DeclarationName.h"
#include "clang/AST/ExprCXX.h"
Index: clang/include/clang/AST/ASTImportError.h
===
--- clang/include/clang/AST/ASTImportError.h
+++ clang/include/clang/AST/ASTImportError.h
@@ -1,4 +1,5 @@
-//===- ASTImporter.h - Import Error while Importing AST ---*- C++ -*-===//
+//===- ASTImportError.h - Define errors while importing AST ---*- C++
+//-*-===//
//
// Part of the LLVM Project, under the Apache License v2.0 with LLVM
Exceptions.
// See https://llvm.org/LICENSE.txt for license information.
@@ -14,7 +15,6 @@
#ifndef LLVM_CLANG_AST_ASTIMPORTERROR_H
#define LLVM_CLANG_AST_ASTIMPORTERROR_H
-#include "clang/AST/APValue.h"
#include "llvm/Support/Error.h"
namespace clang {
@@ -42,10 +42,10 @@
std::string toString() const;
- void log(raw_ostream &OS) const override;
+ void log(llvm::raw_ostream &OS) const override;
std::error_code convertToErrorCode() const override;
};
} // namespace clang
-#endif // LLVM_CLANG_AST_ASTIMPORTERROR_H
\ No newline at end of file
+#endif // LLVM_CLANG_AST_ASTIMPORTERROR_H
Index: clang/include/clang/AST/ASTImporterSharedState.h
===
--- clang/include/clang/AST/ASTImporterSharedState.h
+++ clang/include/clang/AST/ASTImporterSharedState.h
@@ -14,6 +14,7 @@
#ifndef LLVM_CLANG_AST_ASTIMPORTERSHAREDSTATE_H
#define LLVM_CLANG_AST_ASTIMPORTERSHAREDSTATE_H
+#include "clang/AST/ASTImportError.h"
#include "clang/AST/ASTImporterLookupTable.h"
#include "clang/AST/Decl.h"
#include "llvm/ADT/DenseMap.h"
Index: clang/include/clang/AST/ASTImporterLookupTable.h
===
--- clang/include/clang/AST/ASTImporterLookupTable.h
+++ clang/include/clang/AST/ASTImporterLookupTable.h
@@ -14,7 +14,6 @@
#ifndef LLVM_CLANG_AST_ASTIMPORTERLOOKUPTABLE_H
#define LLVM_CLANG_AST_ASTIMPORTERLOOKUPTABLE_H
-#include "clang/AST/ASTImportError.h"
#include "clang/AST/DeclBase.h" // lookup_result
#include "clang/AST/DeclarationName.h"
#include "llvm/ADT/DenseMap.h"
Index: clang/include/clang/AST/ASTImporter.h
===
--- clang/include/clang/AST/ASTImporter.h
+++ clang/include/clang/AST/ASTImporter.h
@@ -14,7 +14,7 @@
#ifndef LLVM_CLANG_AST_ASTIMPORTER_H
#define LLVM_CLANG_AST_ASTIMPORTER_H
-#include "clang/AST/ASTImporterLookupTable.h"
+#include "clang/AST/ASTImportError.h"
#include "clang/AST/DeclBase.h"
#include "clang/AST/DeclarationName.h"
#include "clang/AST/ExprCXX.h"
Index: clang/include/clang/AST/ASTImportError.h
===
--- clang/include/clang/AST/ASTImportError.h
+++ clang/include/clang/AST/ASTImportError.h
@@ -1,4 +1,5 @@
-//===- ASTImporter.h - Import Error while Importing AST ---*- C++ -*-===//
+//===- ASTImportError.h - Define errors while importing AST ---*- C++
+//-*-===//
//
// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
// See https://llvm.or
