[clang][ASTImporter][NFC]: Move clang::ImportError into own header.

[balazske]

The ASTImporterLookupTable was not dependent on the ImportError so the header should not be included in that file. It is better in ASTImporterSharedState.h. Alternative solution is to add class ASTImportError into ASTImporterSharedState.h (no new header is needed) and use this file from ASTImporter.h.

clang/include/clang/AST/ASTImportError.h

2

23

Rename to ASTImportError.

clang/include/clang/AST/ASTImporter.h

17

Include ASTImportError.h.

clang/include/clang/AST/ASTImporterLookupTable.h

17 ↗

(On Diff #426429)

This include is not needed here.

[balazske]

A better title: "Move clang::ImportError into own header and rename it (NFC)."

[phyBrackets]

clang/include/clang/AST/ASTImporterLookupTable.h

17 ↗

(On Diff #426429)

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 .

[balazske]

clang/include/clang/AST/ASTImportError.h

18

This header is probably not needed.

23

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).

clang/include/clang/AST/ASTImporterLookupTable.h

17 ↗

(On Diff #426429)

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.

[phyBrackets]

clang/include/clang/AST/ASTImportError.h

23

So, what do you suggest , for this patch should I go with name ImportError only ? And rename it as ASTImportError in a separate patch ?

[phyBrackets]

clang/include/clang/AST/ASTImporterLookupTable.h

17 ↗

(On Diff #426429)

ahh yes! Thanks

[balazske]

clang/include/clang/AST/ASTImportError.h

2

23

Yes: Do not change the class name here (but header should be called ASTImportError.h). In a next patch make only a rename.

51

Please add newline. I do not know if this can cause buildbot failures.

[phyBrackets]

Address inline comments

[balazske]

It looks mostly good now, apart from the line 1 comment problem. Usually in a review all phabricator diffs are created against the original commit or at least against an existing (in the current github repository) commit, here the last diff seems to be against an other commit. The next diff should be based on an existing commit (can be same as the original base). Otherwise the arc patch command may not work (I do not know if you use it) and it is difficult to compare changes related to the original code.

clang/include/clang/AST/ASTImportError.h

2

One small problem: This line 2 is the end of line 1, this should be part of line 1. Line 1 should be exactly 80 characters long.

[phyBrackets]

address inline comment

[phyBrackets]

I'm having a problem , this patch expected to address the previous inline comment but after making the specific change and committing it , when I'm creating the patch using arc diff --update <revisionId> , the changes were suddenly gone and I thought that maybe the changes were there in the commit atleast but I think commit got reverted and there is nothing for diff but suddenly the patch were created .

[phyBrackets]

address inline comment

[phyBrackets]

In D124774#3493382, @phyBrackets wrote:

I'm having a problem , this patch expected to address the previous inline comment but after making the specific change and committing it , when I'm creating the patch using arc diff --update <revisionId> , the changes were suddenly gone and I thought that maybe the changes were there in the commit atleast but I think commit got reverted and there is nothing for diff but suddenly the patch were created .

I think there was the problem with number of the character in line 1 , Now it seems correct .

[phyBrackets]

Correct the number of character in line 1

[phyBrackets]

address the buildbot failing issue

[phyBrackets]

is there still any problem ? I'm not sure ?

[balazske]

The code looks correct but the diff is not OK. I think that you have separate commits for every change (instead of changing the first with git commit --amend when a new change is made). In this case arc diff --update <ID> main may work (if you have a separate branch for the changes). (I did not found a place in LLVM docs where this part of the process is exactly described.)

[martong]

In D124774#3493868, @balazske wrote:

The code looks correct but the diff is not OK. I think that you have separate commits for every change (instead of changing the first with git commit --amend when a new change is made). In this case arc diff --update <ID> main may work (if you have a separate branch for the changes). (I did not found a place in LLVM docs where this part of the process is exactly described.)

Yes, you should see that your baseline is valid. Below, the highlighted commit does not exists in the remote (in the upstream repository).

I suggest the following.

git remote add llvm [e-mail]:llvm/llvm-project.git
git fetch llvm
git rebase -i llvm/main //choose your relevant commits
arc diff --update D124774 llvm/main

[phyBrackets]

Rebase several commits

[martong]

LGTM

[phyBrackets]

Thanks @martong @balazske for the Review .

clang/include/clang/AST/ASTImportError.h

+//===- 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.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+//
+//  This file defines the ASTImportError class which basically defines the kind
+//  of error while importing AST .
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_CLANG_AST_ASTIMPORTERROR_H
+#define LLVM_CLANG_AST_ASTIMPORTERROR_H
+
+#include "llvm/Support/Error.h"
+
+namespace clang {
+
+class ImportError : public llvm::ErrorInfo<ImportError> {
+public:
+  /// \brief Kind of error when importing an AST component.
+  enum ErrorKind {
+    NameConflict,         /// Naming ambiguity (likely ODR violation).
+    UnsupportedConstruct, /// Not supported node or case.
+    Unknown               /// Other error.
+  };
+
+  ErrorKind Error;
+
+  static char ID;
+
+  ImportError() : Error(Unknown) {}
+  ImportError(const ImportError &Other) : Error(Other.Error) {}
+  ImportError &operator=(const ImportError &Other) {
+    Error = Other.Error;
+    return *this;
+  }
+  ImportError(ErrorKind Error) : Error(Error) {}
+
+  std::string toString() const;
+
+  void log(llvm::raw_ostream &OS) const override;
+  std::error_code convertToErrorCode() const override;
+};
+
+} // namespace clang
+
+#endif // LLVM_CLANG_AST_ASTIMPORTERROR_H

clang/include/clang/AST/ASTImporter.h

 //===- ASTImporter.h - Importing ASTs from other Contexts -------*- C++ -*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 //
 //  This file defines the ASTImporter class which imports AST nodes from one
 //  context into another context.
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLVM_CLANG_AST_ASTIMPORTER_H
 #define LLVM_CLANG_AST_ASTIMPORTER_H
 
-#include "clang/AST/APValue.h"
+#include "clang/AST/ASTImportError.h"
 #include "clang/AST/DeclBase.h"
 #include "clang/AST/DeclarationName.h"
 #include "clang/AST/ExprCXX.h"
 #include "clang/AST/NestedNameSpecifier.h"
 #include "clang/AST/TemplateName.h"
 #include "clang/AST/Type.h"
 #include "clang/Basic/Diagnostic.h"
 #include "clang/Basic/IdentifierTable.h"
 #include "clang/Basic/LLVM.h"
 #include "clang/Basic/SourceLocation.h"
 #include "llvm/ADT/DenseMap.h"
 #include "llvm/ADT/DenseSet.h"
 #include "llvm/ADT/Optional.h"
 #include "llvm/ADT/SmallVector.h"
-#include "llvm/Support/Error.h"
 #include <utility>
 
 namespace clang {
 
 class ASTContext;
 class ASTImporterSharedState;
 class Attr;
 class CXXBaseSpecifier;
 class CXXCtorInitializer;
 class Decl;
 class DeclContext;
 class Expr;
 class FileManager;
 class NamedDecl;
 class Stmt;
 class TagDecl;
 class TranslationUnitDecl;
 class TypeSourceInfo;
 
-  class ImportError : public llvm::ErrorInfo<ImportError> {
-  public:
-    /// \brief Kind of error when importing an AST component.
-    enum ErrorKind {
-        NameConflict, /// Naming ambiguity (likely ODR violation).
-        UnsupportedConstruct, /// Not supported node or case.
-        Unknown /// Other error.
-    };
-
-    ErrorKind Error;
-
-    static char ID;
-
-    ImportError() : Error(Unknown) {}
-    ImportError(const ImportError &Other) : Error(Other.Error) {}
-    ImportError &operator=(const ImportError &Other) {
-      Error = Other.Error;
-      return *this;
-    }
-    ImportError(ErrorKind Error) : Error(Error) { }
-
-    std::string toString() const;
-
-    void log(raw_ostream &OS) const override;
-    std::error_code convertToErrorCode() const override;
-  };
-
   // \brief Returns with a list of declarations started from the canonical decl
   // then followed by subsequent decls in the translation unit.
   // This gives a canonical list for each entry in the redecl chain.
   // `Decl::redecls()` gives a list of decls which always start from the
   // previous decl and the next item is actually the previous item in the order
   // of source locations.  Thus, `Decl::redecls()` gives different lists for
   // the different entries in a given redecl chain.
   llvm::SmallVector<Decl*, 2> getCanonicalForwardRedeclChain(Decl* D);

clang/include/clang/AST/ASTImporterSharedState.h

 //===- ASTImporterSharedState.h - ASTImporter specific state --*- C++ -*---===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 //
 //  This file defines the ASTImporter specific state, which may be shared
 //  amongst several ASTImporter objects.
 //
 //===----------------------------------------------------------------------===//
 
 #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"
-// FIXME We need this because of ImportError.
-#include "clang/AST/ASTImporter.h"
 
 namespace clang {
 
 class TranslationUnitDecl;
 
 /// Importer specific state, which may be shared amongst several ASTImporter
 /// objects.
 class ASTImporterSharedState {