[PATCH] D53751: [ASTImporter] Added Import functions for transition to new API.

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D53751#1311037 , @martong wrote:

> > I didn't actually see this comment get addressed other than to say it won't 
> > be a problem in practice, which I'm not certain I agree with. Was there a 
> > reason why this got commit before finding out if the reviewer with the 
> > concern agrees with your rationale? FWIW, I share the concern that having 
> > parallel APIs for any length of time is a dangerous thing. Given that 
> > "almost ready to go" is not "ready to go" but there's not an imminent 
> > release, I don't understand the rush to commit this.
>
> @aaron.ballman Thanks for your time and review on this.
>
> I completely understand you concern and agree that having such parallel API 
> even for a short time is not good. Please let me explain why we chose to do 
> this still:
>  `ASTImporter::Import` functions are used externally by LLDB and CTU as 
> clients. However, the functions are used internally too, inside `ASTImporter` 
> and `ASTNodeImporter`.  E.g. when we import an expression, then first we use 
> the `Import(QualType)` function to import its type.
>  Our goal is twofold: (1) enforce `ASTImporter` and `ASTNodeImporter` 
> implementation functions to always check the result of used import functions 
> and (2) make sure that clients can have detailed error information, so e.g. 
> in case of CTU we can handle unsupported constructs differently than ODR 
> errors.
>  As @balazske mentioned we could have changed the API in one lockstep but the 
> impact would have been too huge.
>
> In the context of this patch, you can think of the newly introduced 
> `Import_New` functions as the internal only functions. I was thinking about 
> that we could make them private and `ASTNodeImporter` as a friend,  that way 
> we could hide them from clients and then the dual API would cease to exist.


Thank you for the explanation -- I guess I would have preferred to go with the 
suggestion from @shafik to have done this one API at a time, rather than as an 
entire set of APIs. However, given that the code is already in, I don't think 
it would be worth the churn to revert and go that route.


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D53751/new/

https://reviews.llvm.org/D53751



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D53751: [ASTImporter] Added Import functions for transition to new API.

[Shafik Yaghmour via Phabricator via cfe-commits]

shafik added a comment.

In D53751#1311037 , @martong wrote:

> > I didn't actually see this comment get addressed other than to say it won't 
> > be a problem in practice, which I'm not certain I agree with. Was there a 
> > reason why this got commit before finding out if the reviewer with the 
> > concern agrees with your rationale? FWIW, I share the concern that having 
> > parallel APIs for any length of time is a dangerous thing. Given that 
> > "almost ready to go" is not "ready to go" but there's not an imminent 
> > release, I don't understand the rush to commit this.
>
> @aaron.ballman Thanks for your time and review on this.
>
> I completely understand you concern and agree that having such parallel API 
> even for a short time is not good. Please let me explain why we chose to do 
> this still:
>  `ASTImporter::Import` functions are used externally by LLDB and CTU as 
> clients. However, the functions are used internally too, inside `ASTImporter` 
> and `ASTNodeImporter`.  E.g. when we import an expression, then first we use 
> the `Import(QualType)` function to import its type.
>  Our goal is twofold: (1) enforce `ASTImporter` and `ASTNodeImporter` 
> implementation functions to always check the result of used import functions 
> and (2) make sure that clients can have detailed error information, so e.g. 
> in case of CTU we can handle unsupported constructs differently than ODR 
> errors.
>  As @balazske mentioned we could have changed the API in one lockstep but the 
> impact would have been too huge.


I believe an alternate approach would have been to change one function at a 
time and do this change across. These would have been smaller changes and 
easier to review. This would have prevented the need for an intermediate name 
which causes churn in the commit history.

> In the context of this patch, you can think of the newly introduced 
> `Import_New` functions as the internal only functions. I was thinking about 
> that we could make them private and `ASTNodeImporter` as a friend,  that way 
> we could hide them from clients and then the dual API would cease to exist.


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D53751/new/

https://reviews.llvm.org/D53751



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D53751: [ASTImporter] Added Import functions for transition to new API.

[Gabor Marton via Phabricator via cfe-commits]

martong added a comment.

> I didn't actually see this comment get addressed other than to say it won't 
> be a problem in practice, which I'm not certain I agree with. Was there a 
> reason why this got commit before finding out if the reviewer with the 
> concern agrees with your rationale? FWIW, I share the concern that having 
> parallel APIs for any length of time is a dangerous thing. Given that "almost 
> ready to go" is not "ready to go" but there's not an imminent release, I 
> don't understand the rush to commit this.

@aaron.ballman Thanks for your time and review on this.

I completely understand you concern and agree that having such parallel API 
even for a short time is not good. Please let me explain why we chose to do 
this still:
`ASTImporter::Import` functions are used externally by LLDB and CTU as clients. 
However, the functions are used internally too, inside `ASTImporter` and 
`ASTNodeImporter`.  E.g. when we import an expression, then first we use the 
`Import(QualType)` function to import its type.
Our goal is twofold: (1) enforce `ASTImporter` and `ASTNodeImporter` 
implementation functions to always check the result of used import functions 
and (2) make sure that clients can have detailed error information, so e.g. in 
case of CTU we can handle unsupported constructs differently than ODR errors.
As @balazske mentioned we could have changed the API in one lockstep but the 
impact would have been too huge.

In the context of this patch, you can think of the newly introduced 
`Import_New` functions as the internal only functions. I was thinking about 
that we could make them private and `ASTNodeImporter` as a friend,  that way we 
could hide them from clients and then the dual API would cease to exist.


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D53751/new/

https://reviews.llvm.org/D53751



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D53751: [ASTImporter] Added Import functions for transition to new API.

[Balázs Kéri via Phabricator via cfe-commits]

balazske added a comment.

It is really "ugly" thing to have a Import_New and a Import for the same 
functionality. Without these a single large patch would have been needed for 
clang and another smaller one for LLDB that must be committed "at the same 
time". Now 1 dependent patch is under review. Then 1 patch is to do for clang 
and one for lldb (these can contain the removal of Import_New too). It mainly 
depends on the speed of the reviews how fast the update happens.


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D53751/new/

https://reviews.llvm.org/D53751



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D53751: [ASTImporter] Added Import functions for transition to new API.

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D53751#1301551 , @shafik wrote:

> I think these changes make sense at a high level but I am not sure about the 
> refactoring strategy. I am especially concerned we may end up in place where 
> all the effected users of the API don't get updated and we are stuck with 
> this parallel API.


I didn't actually see this comment get addressed other than to say it won't be 
a problem in practice, which I'm not certain I agree with. Was there a reason 
why this got commit before finding out if the reviewer with the concern agrees 
with your rationale? FWIW, I share the concern that having parallel APIs for 
any length of time is a dangerous thing. Given that "almost ready to go" is not 
"ready to go" but there's not an imminent release, I don't understand the rush 
to commit this.

When is the renaming and removal of the old API expected take place? Days? 
Weeks? Months?


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D53751/new/

https://reviews.llvm.org/D53751



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D53751: [ASTImporter] Added Import functions for transition to new API.

[Balázs Kéri via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL347685: [ASTImporter] Added Import functions for transition 
to new API. (authored by balazske, committed by ).
Herald added a subscriber: llvm-commits.

Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D53751/new/

https://reviews.llvm.org/D53751

Files:
  cfe/trunk/include/clang/AST/ASTImporter.h
  cfe/trunk/lib/AST/ASTImporter.cpp

Index: cfe/trunk/include/clang/AST/ASTImporter.h
===
--- cfe/trunk/include/clang/AST/ASTImporter.h
+++ cfe/trunk/include/clang/AST/ASTImporter.h
@@ -166,30 +166,41 @@
 }
 
 /// Import the given type from the "from" context into the "to"
-/// context.
+/// context. A null type is imported as a null type (no error).
 ///
-/// \returns the equivalent type in the "to" context, or a NULL type if
-/// an error occurred.
+/// \returns The equivalent type in the "to" context, or the import error.
+llvm::Expected Import_New(QualType FromT);
+// FIXME: Remove this version.
 QualType Import(QualType FromT);
 
 /// Import the given type source information from the
 /// "from" context into the "to" context.
 ///
-/// \returns the equivalent type source information in the "to"
-/// context, or NULL if an error occurred.
+/// \returns The equivalent type source information in the "to"
+/// context, or the import error.
+llvm::Expected Import_New(TypeSourceInfo *FromTSI);
+// FIXME: Remove this version.
 TypeSourceInfo *Import(TypeSourceInfo *FromTSI);
 
 /// Import the given attribute from the "from" context into the
 /// "to" context.
 ///
-/// \returns the equivalent attribute in the "to" context.
+/// \returns The equivalent attribute in the "to" context, or the import
+/// error.
+llvm::Expected Import_New(const Attr *FromAttr);
+// FIXME: Remove this version.
 Attr *Import(const Attr *FromAttr);
 
 /// Import the given declaration from the "from" context into the
 /// "to" context.
 ///
-/// \returns the equivalent declaration in the "to" context, or a NULL type
-/// if an error occurred.
+/// \returns The equivalent declaration in the "to" context, or the import
+/// error.
+llvm::Expected Import_New(Decl *FromD);
+llvm::Expected Import_New(const Decl *FromD) {
+  return Import_New(const_cast(FromD));
+}
+// FIXME: Remove this version.
 Decl *Import(Decl *FromD);
 Decl *Import(const Decl *FromD) {
   return Import(const_cast(FromD));
@@ -210,87 +221,117 @@
 /// Import the given expression from the "from" context into the
 /// "to" context.
 ///
-/// \returns the equivalent expression in the "to" context, or NULL if
-/// an error occurred.
+/// \returns The equivalent expression in the "to" context, or the import
+/// error.
+llvm::Expected Import_New(Expr *FromE);
+// FIXME: Remove this version.
 Expr *Import(Expr *FromE);
 
 /// Import the given statement from the "from" context into the
 /// "to" context.
 ///
-/// \returns the equivalent statement in the "to" context, or NULL if
-/// an error occurred.
+/// \returns The equivalent statement in the "to" context, or the import
+/// error.
+llvm::Expected Import_New(Stmt *FromS);
+// FIXME: Remove this version.
 Stmt *Import(Stmt *FromS);
 
 /// Import the given nested-name-specifier from the "from"
 /// context into the "to" context.
 ///
-/// \returns the equivalent nested-name-specifier in the "to"
-/// context, or NULL if an error occurred.
+/// \returns The equivalent nested-name-specifier in the "to"
+/// context, or the import error.
+llvm::Expected
+Import_New(NestedNameSpecifier *FromNNS);
+// FIXME: Remove this version.
 NestedNameSpecifier *Import(NestedNameSpecifier *FromNNS);
 
-/// Import the given nested-name-specifier from the "from"
+/// Import the given nested-name-specifier-loc from the "from"
 /// context into the "to" context.
 ///
-/// \returns the equivalent nested-name-specifier in the "to"
-/// context.
+/// \returns The equivalent nested-name-specifier-loc in the "to"
+/// context, or the import error.
+llvm::Expected
+Import_New(NestedNameSpecifierLoc FromNNS);
+// FIXME: Remove this version.
 NestedNameSpecifierLoc Import(NestedNameSpecifierLoc FromNNS);
 
-/// Import the goven template name from the "from" context into the
-/// "to" context.
+/// Import the given template name from the "from" context into the
+/// "to" context, or the import error.
+llvm::Expected Import_New(TemplateName From);
+// FIXME: Remove this version.
 TemplateName Import(TemplateName From);
 
 /// Import the given source location from the "from" context into
 /// the 

[PATCH] D53751: [ASTImporter] Added Import functions for transition to new API.

[Balázs Kéri via Phabricator via cfe-commits]

balazske updated this revision to Diff 175529.
balazske added a comment.

- Corrected Import_New(const Attr *)
- Added missing Import_New with Selector and DeclarationName.
- Split long lines.
- Split long lines (ASTImporter.cpp).

Rebase to newest master.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D53751/new/

https://reviews.llvm.org/D53751

Files:
  include/clang/AST/ASTImporter.h
  lib/AST/ASTImporter.cpp

Index: lib/AST/ASTImporter.cpp
===
--- lib/AST/ASTImporter.cpp
+++ lib/AST/ASTImporter.cpp
@@ -7683,6 +7683,12 @@
 
 ASTImporter::~ASTImporter() = default;
 
+Expected ASTImporter::Import_New(QualType FromT) {
+  QualType ToT = Import(FromT);
+  if (ToT.isNull() && !FromT.isNull())
+return make_error();
+  return ToT;
+}
 QualType ASTImporter::Import(QualType FromT) {
   if (FromT.isNull())
 return {};
@@ -7709,6 +7715,12 @@
   return ToContext.getQualifiedType(*ToTOrErr, FromT.getLocalQualifiers());
 }
 
+Expected ASTImporter::Import_New(TypeSourceInfo *FromTSI) {
+  TypeSourceInfo *ToTSI = Import(FromTSI);
+  if (!ToTSI && FromTSI)
+return llvm::make_error();
+  return ToTSI;
+}
 TypeSourceInfo *ASTImporter::Import(TypeSourceInfo *FromTSI) {
   if (!FromTSI)
 return FromTSI;
@@ -7723,8 +7735,12 @@
   T, Import(FromTSI->getTypeLoc().getBeginLoc()));
 }
 
+Expected ASTImporter::Import_New(const Attr *FromAttr) {
+  return Import(FromAttr);
+}
 Attr *ASTImporter::Import(const Attr *FromAttr) {
   Attr *ToAttr = FromAttr->clone(ToContext);
+  // NOTE: Import of SourceRange may fail.
   ToAttr->setRange(Import(FromAttr->getRange()));
   return ToAttr;
 }
@@ -7742,6 +7758,12 @@
   }
 }
 
+Expected ASTImporter::Import_New(Decl *FromD) {
+  Decl *ToD = Import(FromD);
+  if (!ToD && FromD)
+return llvm::make_error();
+  return ToD;
+}
 Decl *ASTImporter::Import(Decl *FromD) {
   if (!FromD)
 return nullptr;
@@ -7830,6 +7852,12 @@
   return ToDC;
 }
 
+Expected ASTImporter::Import_New(Expr *FromE) {
+  Expr *ToE = Import(FromE);
+  if (!ToE && FromE)
+return llvm::make_error();
+  return ToE;
+}
 Expr *ASTImporter::Import(Expr *FromE) {
   if (!FromE)
 return nullptr;
@@ -7837,6 +7865,12 @@
   return cast_or_null(Import(cast(FromE)));
 }
 
+Expected ASTImporter::Import_New(Stmt *FromS) {
+  Stmt *ToS = Import(FromS);
+  if (!ToS && FromS)
+return llvm::make_error();
+  return ToS;
+}
 Stmt *ASTImporter::Import(Stmt *FromS) {
   if (!FromS)
 return nullptr;
@@ -7872,6 +7906,13 @@
   return *ToSOrErr;
 }
 
+Expected
+ASTImporter::Import_New(NestedNameSpecifier *FromNNS) {
+  NestedNameSpecifier *ToNNS = Import(FromNNS);
+  if (!ToNNS && FromNNS)
+return llvm::make_error();
+  return ToNNS;
+}
 NestedNameSpecifier *ASTImporter::Import(NestedNameSpecifier *FromNNS) {
   if (!FromNNS)
 return nullptr;
@@ -7925,6 +7966,11 @@
   llvm_unreachable("Invalid nested name specifier kind");
 }
 
+Expected
+ASTImporter::Import_New(NestedNameSpecifierLoc FromNNS) {
+  NestedNameSpecifierLoc ToNNS = Import(FromNNS);
+  return ToNNS;
+}
 NestedNameSpecifierLoc ASTImporter::Import(NestedNameSpecifierLoc FromNNS) {
   // Copied from NestedNameSpecifier mostly.
   SmallVector NestedNames;
@@ -7996,6 +8042,12 @@
   return Builder.getWithLocInContext(getToContext());
 }
 
+Expected ASTImporter::Import_New(TemplateName From) {
+  TemplateName To = Import(From);
+  if (To.isNull() && !From.isNull())
+return llvm::make_error();
+  return To;
+}
 TemplateName ASTImporter::Import(TemplateName From) {
   switch (From.getKind()) {
   case TemplateName::Template:
@@ -8086,6 +8138,12 @@
   llvm_unreachable("Invalid template name kind");
 }
 
+Expected ASTImporter::Import_New(SourceLocation FromLoc) {
+  SourceLocation ToLoc = Import(FromLoc);
+  if (ToLoc.isInvalid() && !FromLoc.isInvalid())
+return llvm::make_error();
+  return ToLoc;
+}
 SourceLocation ASTImporter::Import(SourceLocation FromLoc) {
   if (FromLoc.isInvalid())
 return {};
@@ -8100,10 +8158,20 @@
   return ToSM.getComposedLoc(ToFileID, Decomposed.second);
 }
 
+Expected ASTImporter::Import_New(SourceRange FromRange) {
+  SourceRange ToRange = Import(FromRange);
+  return ToRange;
+}
 SourceRange ASTImporter::Import(SourceRange FromRange) {
   return SourceRange(Import(FromRange.getBegin()), Import(FromRange.getEnd()));
 }
 
+Expected ASTImporter::Import_New(FileID FromID) {
+  FileID ToID = Import(FromID);
+  if (ToID.isInvalid() && FromID.isValid())
+return llvm::make_error();
+  return ToID;
+}
 FileID ASTImporter::Import(FileID FromID) {
   llvm::DenseMap::iterator Pos = ImportedFileIDs.find(FromID);
   if (Pos != ImportedFileIDs.end())
@@ -8161,6 +8229,13 @@
   return ToID;
 }
 
+Expected
+ASTImporter::Import_New(CXXCtorInitializer *From) {
+  CXXCtorInitializer *To = Import(From);
+  if (!To && From)
+return llvm::make_error();
+  return To;
+}
 CXXCtorInitializer 

[PATCH] D53751: [ASTImporter] Added Import functions for transition to new API.

[Aleksei Sidorin via Phabricator via cfe-commits]

a_sidorin accepted this revision.
a_sidorin added a comment.
This revision is now accepted and ready to land.

LGTM, thanks!


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D53751/new/

https://reviews.llvm.org/D53751



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D53751: [ASTImporter] Added Import functions for transition to new API.

[Balázs Kéri via Phabricator via cfe-commits]

balazske updated this revision to Diff 175212.
balazske marked an inline comment as done.
balazske added a comment.

- Split long lines (ASTImporter.cpp).


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D53751/new/

https://reviews.llvm.org/D53751

Files:
  include/clang/AST/ASTImporter.h
  lib/AST/ASTImporter.cpp

Index: lib/AST/ASTImporter.cpp
===
--- lib/AST/ASTImporter.cpp
+++ lib/AST/ASTImporter.cpp
@@ -7656,6 +7656,12 @@
 
 ASTImporter::~ASTImporter() = default;
 
+Expected ASTImporter::Import_New(QualType FromT) {
+  QualType ToT = Import(FromT);
+  if (ToT.isNull() && !FromT.isNull())
+return make_error();
+  return ToT;
+}
 QualType ASTImporter::Import(QualType FromT) {
   if (FromT.isNull())
 return {};
@@ -7682,6 +7688,12 @@
   return ToContext.getQualifiedType(*ToTOrErr, FromT.getLocalQualifiers());
 }
 
+Expected ASTImporter::Import_New(TypeSourceInfo *FromTSI) {
+  TypeSourceInfo *ToTSI = Import(FromTSI);
+  if (!ToTSI && FromTSI)
+return llvm::make_error();
+  return ToTSI;
+}
 TypeSourceInfo *ASTImporter::Import(TypeSourceInfo *FromTSI) {
   if (!FromTSI)
 return FromTSI;
@@ -7696,8 +7708,12 @@
   T, Import(FromTSI->getTypeLoc().getBeginLoc()));
 }
 
+Expected ASTImporter::Import_New(const Attr *FromAttr) {
+  return Import(FromAttr);
+}
 Attr *ASTImporter::Import(const Attr *FromAttr) {
   Attr *ToAttr = FromAttr->clone(ToContext);
+  // NOTE: Import of SourceRange may fail.
   ToAttr->setRange(Import(FromAttr->getRange()));
   return ToAttr;
 }
@@ -7715,6 +7731,12 @@
   }
 }
 
+Expected ASTImporter::Import_New(Decl *FromD) {
+  Decl *ToD = Import(FromD);
+  if (!ToD && FromD)
+return llvm::make_error();
+  return ToD;
+}
 Decl *ASTImporter::Import(Decl *FromD) {
   if (!FromD)
 return nullptr;
@@ -7803,6 +7825,12 @@
   return ToDC;
 }
 
+Expected ASTImporter::Import_New(Expr *FromE) {
+  Expr *ToE = Import(FromE);
+  if (!ToE && FromE)
+return llvm::make_error();
+  return ToE;
+}
 Expr *ASTImporter::Import(Expr *FromE) {
   if (!FromE)
 return nullptr;
@@ -7810,6 +7838,12 @@
   return cast_or_null(Import(cast(FromE)));
 }
 
+Expected ASTImporter::Import_New(Stmt *FromS) {
+  Stmt *ToS = Import(FromS);
+  if (!ToS && FromS)
+return llvm::make_error();
+  return ToS;
+}
 Stmt *ASTImporter::Import(Stmt *FromS) {
   if (!FromS)
 return nullptr;
@@ -7845,6 +7879,13 @@
   return *ToSOrErr;
 }
 
+Expected
+ASTImporter::Import_New(NestedNameSpecifier *FromNNS) {
+  NestedNameSpecifier *ToNNS = Import(FromNNS);
+  if (!ToNNS && FromNNS)
+return llvm::make_error();
+  return ToNNS;
+}
 NestedNameSpecifier *ASTImporter::Import(NestedNameSpecifier *FromNNS) {
   if (!FromNNS)
 return nullptr;
@@ -7898,6 +7939,11 @@
   llvm_unreachable("Invalid nested name specifier kind");
 }
 
+Expected
+ASTImporter::Import_New(NestedNameSpecifierLoc FromNNS) {
+  NestedNameSpecifierLoc ToNNS = Import(FromNNS);
+  return ToNNS;
+}
 NestedNameSpecifierLoc ASTImporter::Import(NestedNameSpecifierLoc FromNNS) {
   // Copied from NestedNameSpecifier mostly.
   SmallVector NestedNames;
@@ -7969,6 +8015,12 @@
   return Builder.getWithLocInContext(getToContext());
 }
 
+Expected ASTImporter::Import_New(TemplateName From) {
+  TemplateName To = Import(From);
+  if (To.isNull() && !From.isNull())
+return llvm::make_error();
+  return To;
+}
 TemplateName ASTImporter::Import(TemplateName From) {
   switch (From.getKind()) {
   case TemplateName::Template:
@@ -8059,6 +8111,12 @@
   llvm_unreachable("Invalid template name kind");
 }
 
+Expected ASTImporter::Import_New(SourceLocation FromLoc) {
+  SourceLocation ToLoc = Import(FromLoc);
+  if (ToLoc.isInvalid() && !FromLoc.isInvalid())
+return llvm::make_error();
+  return ToLoc;
+}
 SourceLocation ASTImporter::Import(SourceLocation FromLoc) {
   if (FromLoc.isInvalid())
 return {};
@@ -8073,10 +8131,20 @@
   return ToSM.getComposedLoc(ToFileID, Decomposed.second);
 }
 
+Expected ASTImporter::Import_New(SourceRange FromRange) {
+  SourceRange ToRange = Import(FromRange);
+  return ToRange;
+}
 SourceRange ASTImporter::Import(SourceRange FromRange) {
   return SourceRange(Import(FromRange.getBegin()), Import(FromRange.getEnd()));
 }
 
+Expected ASTImporter::Import_New(FileID FromID) {
+  FileID ToID = Import(FromID);
+  if (ToID.isInvalid() && FromID.isValid())
+return llvm::make_error();
+  return ToID;
+}
 FileID ASTImporter::Import(FileID FromID) {
   llvm::DenseMap::iterator Pos = ImportedFileIDs.find(FromID);
   if (Pos != ImportedFileIDs.end())
@@ -8134,6 +8202,13 @@
   return ToID;
 }
 
+Expected
+ASTImporter::Import_New(CXXCtorInitializer *From) {
+  CXXCtorInitializer *To = Import(From);
+  if (!To && From)
+return llvm::make_error();
+  return To;
+}
 CXXCtorInitializer *ASTImporter::Import(CXXCtorInitializer *From) {
   Expr *ToExpr = Import(From->getInit());
   if (!ToExpr && From->getInit())
@@ -8179,6 

[PATCH] D53751: [ASTImporter] Added Import functions for transition to new API.

[Balázs Kéri via Phabricator via cfe-commits]

balazske updated this revision to Diff 175207.
balazske added a comment.

- Split long lines.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D53751/new/

https://reviews.llvm.org/D53751

Files:
  include/clang/AST/ASTImporter.h
  lib/AST/ASTImporter.cpp

Index: lib/AST/ASTImporter.cpp
===
--- lib/AST/ASTImporter.cpp
+++ lib/AST/ASTImporter.cpp
@@ -7656,6 +7656,12 @@
 
 ASTImporter::~ASTImporter() = default;
 
+Expected ASTImporter::Import_New(QualType FromT) {
+  QualType ToT = Import(FromT);
+  if (ToT.isNull() && !FromT.isNull())
+return make_error();
+  return ToT;
+}
 QualType ASTImporter::Import(QualType FromT) {
   if (FromT.isNull())
 return {};
@@ -7682,6 +7688,12 @@
   return ToContext.getQualifiedType(*ToTOrErr, FromT.getLocalQualifiers());
 }
 
+Expected ASTImporter::Import_New(TypeSourceInfo *FromTSI) {
+  TypeSourceInfo *ToTSI = Import(FromTSI);
+  if (!ToTSI && FromTSI)
+return llvm::make_error();
+  return ToTSI;
+}
 TypeSourceInfo *ASTImporter::Import(TypeSourceInfo *FromTSI) {
   if (!FromTSI)
 return FromTSI;
@@ -7696,8 +7708,12 @@
   T, Import(FromTSI->getTypeLoc().getBeginLoc()));
 }
 
+Expected ASTImporter::Import_New(const Attr *FromAttr) {
+  return Import(FromAttr);
+}
 Attr *ASTImporter::Import(const Attr *FromAttr) {
   Attr *ToAttr = FromAttr->clone(ToContext);
+  // NOTE: Import of SourceRange may fail.
   ToAttr->setRange(Import(FromAttr->getRange()));
   return ToAttr;
 }
@@ -7715,6 +7731,12 @@
   }
 }
 
+Expected ASTImporter::Import_New(Decl *FromD) {
+  Decl *ToD = Import(FromD);
+  if (!ToD && FromD)
+return llvm::make_error();
+  return ToD;
+}
 Decl *ASTImporter::Import(Decl *FromD) {
   if (!FromD)
 return nullptr;
@@ -7803,6 +7825,12 @@
   return ToDC;
 }
 
+Expected ASTImporter::Import_New(Expr *FromE) {
+  Expr *ToE = Import(FromE);
+  if (!ToE && FromE)
+return llvm::make_error();
+  return ToE;
+}
 Expr *ASTImporter::Import(Expr *FromE) {
   if (!FromE)
 return nullptr;
@@ -7810,6 +7838,12 @@
   return cast_or_null(Import(cast(FromE)));
 }
 
+Expected ASTImporter::Import_New(Stmt *FromS) {
+  Stmt *ToS = Import(FromS);
+  if (!ToS && FromS)
+return llvm::make_error();
+  return ToS;
+}
 Stmt *ASTImporter::Import(Stmt *FromS) {
   if (!FromS)
 return nullptr;
@@ -7845,6 +7879,12 @@
   return *ToSOrErr;
 }
 
+Expected ASTImporter::Import_New(NestedNameSpecifier *FromNNS) {
+  NestedNameSpecifier *ToNNS = Import(FromNNS);
+  if (!ToNNS && FromNNS)
+return llvm::make_error();
+  return ToNNS;
+}
 NestedNameSpecifier *ASTImporter::Import(NestedNameSpecifier *FromNNS) {
   if (!FromNNS)
 return nullptr;
@@ -7898,6 +7938,10 @@
   llvm_unreachable("Invalid nested name specifier kind");
 }
 
+Expected ASTImporter::Import_New(NestedNameSpecifierLoc FromNNS) {
+  NestedNameSpecifierLoc ToNNS = Import(FromNNS);
+  return ToNNS;
+}
 NestedNameSpecifierLoc ASTImporter::Import(NestedNameSpecifierLoc FromNNS) {
   // Copied from NestedNameSpecifier mostly.
   SmallVector NestedNames;
@@ -7969,6 +8013,12 @@
   return Builder.getWithLocInContext(getToContext());
 }
 
+Expected ASTImporter::Import_New(TemplateName From) {
+  TemplateName To = Import(From);
+  if (To.isNull() && !From.isNull())
+return llvm::make_error();
+  return To;
+}
 TemplateName ASTImporter::Import(TemplateName From) {
   switch (From.getKind()) {
   case TemplateName::Template:
@@ -8059,6 +8109,12 @@
   llvm_unreachable("Invalid template name kind");
 }
 
+Expected ASTImporter::Import_New(SourceLocation FromLoc) {
+  SourceLocation ToLoc = Import(FromLoc);
+  if (ToLoc.isInvalid() && !FromLoc.isInvalid())
+return llvm::make_error();
+  return ToLoc;
+}
 SourceLocation ASTImporter::Import(SourceLocation FromLoc) {
   if (FromLoc.isInvalid())
 return {};
@@ -8073,10 +8129,20 @@
   return ToSM.getComposedLoc(ToFileID, Decomposed.second);
 }
 
+Expected ASTImporter::Import_New(SourceRange FromRange) {
+  SourceRange ToRange = Import(FromRange);
+  return ToRange;
+}
 SourceRange ASTImporter::Import(SourceRange FromRange) {
   return SourceRange(Import(FromRange.getBegin()), Import(FromRange.getEnd()));
 }
 
+Expected ASTImporter::Import_New(FileID FromID) {
+  FileID ToID = Import(FromID);
+  if (ToID.isInvalid() && FromID.isValid())
+return llvm::make_error();
+  return ToID;
+}
 FileID ASTImporter::Import(FileID FromID) {
   llvm::DenseMap::iterator Pos = ImportedFileIDs.find(FromID);
   if (Pos != ImportedFileIDs.end())
@@ -8134,6 +8200,12 @@
   return ToID;
 }
 
+Expected ASTImporter::Import_New(CXXCtorInitializer *From) {
+  CXXCtorInitializer *To = Import(From);
+  if (!To && From)
+return llvm::make_error();
+  return To;
+}
 CXXCtorInitializer *ASTImporter::Import(CXXCtorInitializer *From) {
   Expr *ToExpr = Import(From->getInit());
   if (!ToExpr && From->getInit())
@@ -8179,6 +8251,12 @@
   }
 }
 
+Expected ASTImporter::Import_New(const 

[PATCH] D53751: [ASTImporter] Added Import functions for transition to new API.

[Aleksei Sidorin via Phabricator via cfe-commits]

a_sidorin added a subscriber: spyffe.
a_sidorin added a comment.

Hi Balazs,
 The changes look mostly fine to me. I think they can be accepted after some 
minor stylish fixes.

Hi Shafik,

> I think these changes make sense at a high level but I am not sure about the 
> refactoring strategy. I am especially concerned we may end up in place where 
> all the effected users of the API don't get updated and we are stuck with 
> this parallel API.

As I understand, these changes are done exactly in order to perform a seamless 
transition into a new error-checking API. This was discussed in both cfe-dev 
and lldb-dev: https://lists.llvm.org/pipermail/cfe-dev/2018-April/057771.html.
It looks like the patches for the final transition will be ready soon. 
@balazske, @martong Am I correct?

> Tagging in @rsmith since he has reviewed a lot of recent changes involving 
> ASTImpoter that I have seen recently and he will have a better feeling for 
> how these types of refactoring on handled on the clang side. I am mostly 
> focused on the lldb side but care about the ASTImporter since we depend on it.

Review from @rsmith is extremely appreciated; however, me and @spyffe have 
reviewed most ASTImporter-related patches in the last two years. ASTImporter 
itself doesn't have any use in clang frontend. It was an LLDB bridge only until 
CSA started to use it too.




Comment at: include/clang/AST/ASTImporter.h:244
+/// context, or the import error.
+llvm::Expected Import_New(NestedNameSpecifier 
*FromNNS);
+// FIXME: Remove this version.

Please split this line and the changed line below.



Comment at: lib/AST/ASTImporter.cpp:7882
 
+Expected ASTImporter::Import_New(NestedNameSpecifier 
*FromNNS) {
+  NestedNameSpecifier *ToNNS = Import(FromNNS);

Looks like this line needs to be split too.



Comment at: lib/AST/ASTImporter.cpp:8254
 
+Expected ASTImporter::Import_New(const CXXBaseSpecifier 
*From) {
+  CXXBaseSpecifier *To = Import(From);

... and this one too.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D53751/new/

https://reviews.llvm.org/D53751



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D53751: [ASTImporter] Added Import functions for transition to new API.

[Gabor Marton via Phabricator via cfe-commits]

martong added a subscriber: a_sidorin.
martong added a comment.

> I think these changes make sense at a high level but I am not sure about the 
> refactoring strategy. I am especially concerned we may end up in place where 
> all the effected users of the API don't get updated and we are stuck with 
> this parallel API.
>  Tagging in @rsmith since he has reviewed a lot of recent changes involving 
> ASTImpoter that I have seen recently and he will have a better feeling for 
> how these types of refactoring on handled on the clang side. I am mostly 
> focused on the lldb side but care about the ASTImporter since we depend on it.

Hi @shafik ,

I completely understand your concern. We modify ASTImporter in order to make 
cross translation unit (CTU) analysis working on C++ projects. During these 
modifications we carefully try to keep LLDB functionality intact.

This patch is one of the last patches of a refactor in ASTImporter about using 
`Error` and `Expected` as return types. We need this in CTU analysis because

- we want to distinguish between different kind of errors
- internally in ASTImporter we'd like to enforce the checking whether there has 
been any error during the import of any subexpressions

After this patch our next patch would rename these `Import_New` functions to 
`Import` and the old `Import` function implementations returning with a raw 
pointer would be deleted. This indeed would effect LLDB, thus we are going to 
create an LLDB patch too. We already have that LLDB change in our fork 
(https://github.com/Ericsson/lldb/commit/7085d20) and soon we will put that in 
Phabricator too.

Now, my concern is that waiting for the approve from @rsmith could take several 
months since he is usually very overwhelmed. Unfortunately, we have several 
other changes which depend on this change, so this is a blocker for us. Also, I 
think that @a_sidorin has the greatest experience in ASTImporter code. Would it 
be okay for you to accept this patch once Alexei approved it?


Repository:
  rC Clang

https://reviews.llvm.org/D53751



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D53751: [ASTImporter] Added Import functions for transition to new API.

[Shafik Yaghmour via Phabricator via cfe-commits]

shafik added subscribers: rsmith, shafik.
shafik added a comment.
Herald added a reviewer: shafik.
Herald added a subscriber: gamesh411.

I think these changes make sense at a high level but I am not sure about the 
refactoring strategy. I am especially concerned we may end up in place where 
all the effected users of the API don't get updated and we are stuck with this 
parallel API.

Tagging in @rsmith since he has reviewed a lot of recent changes involving 
ASTImpoter that I have seen recently and he will have a better feeling for how 
these types of refactoring on handled on the clang side. I am mostly focused on 
the lldb side but care about the ASTImporter since we depend on it.


Repository:
  rC Clang

https://reviews.llvm.org/D53751



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D53751: [ASTImporter] Added Import functions for transition to new API.

[Balázs Kéri via Phabricator via cfe-commits]

balazske updated this revision to Diff 171292.
balazske added a comment.

- Added missing Import_New with Selector and DeclarationName.


Repository:
  rC Clang

https://reviews.llvm.org/D53751

Files:
  include/clang/AST/ASTImporter.h
  lib/AST/ASTImporter.cpp

Index: lib/AST/ASTImporter.cpp
===
--- lib/AST/ASTImporter.cpp
+++ lib/AST/ASTImporter.cpp
@@ -7656,6 +7656,12 @@
 
 ASTImporter::~ASTImporter() = default;
 
+Expected ASTImporter::Import_New(QualType FromT) {
+  QualType ToT = Import(FromT);
+  if (ToT.isNull() && !FromT.isNull())
+return make_error();
+  return ToT;
+}
 QualType ASTImporter::Import(QualType FromT) {
   if (FromT.isNull())
 return {};
@@ -7682,6 +7688,12 @@
   return ToContext.getQualifiedType(*ToTOrErr, FromT.getLocalQualifiers());
 }
 
+Expected ASTImporter::Import_New(TypeSourceInfo *FromTSI) {
+  TypeSourceInfo *ToTSI = Import(FromTSI);
+  if (!ToTSI && FromTSI)
+return llvm::make_error();
+  return ToTSI;
+}
 TypeSourceInfo *ASTImporter::Import(TypeSourceInfo *FromTSI) {
   if (!FromTSI)
 return FromTSI;
@@ -7696,8 +7708,12 @@
   T, Import(FromTSI->getTypeLoc().getBeginLoc()));
 }
 
+Expected ASTImporter::Import_New(const Attr *FromAttr) {
+  return Import(FromAttr);
+}
 Attr *ASTImporter::Import(const Attr *FromAttr) {
   Attr *ToAttr = FromAttr->clone(ToContext);
+  // NOTE: Import of SourceRange may fail.
   ToAttr->setRange(Import(FromAttr->getRange()));
   return ToAttr;
 }
@@ -7715,6 +7731,12 @@
   }
 }
 
+Expected ASTImporter::Import_New(Decl *FromD) {
+  Decl *ToD = Import(FromD);
+  if (!ToD && FromD)
+return llvm::make_error();
+  return ToD;
+}
 Decl *ASTImporter::Import(Decl *FromD) {
   if (!FromD)
 return nullptr;
@@ -7803,13 +7825,25 @@
   return ToDC;
 }
 
+Expected ASTImporter::Import_New(Expr *FromE) {
+  Expr *ToE = Import(FromE);
+  if (!ToE && FromE)
+return llvm::make_error();
+  return ToE;
+}
 Expr *ASTImporter::Import(Expr *FromE) {
   if (!FromE)
 return nullptr;
 
   return cast_or_null(Import(cast(FromE)));
 }
 
+Expected ASTImporter::Import_New(Stmt *FromS) {
+  Stmt *ToS = Import(FromS);
+  if (!ToS && FromS)
+return llvm::make_error();
+  return ToS;
+}
 Stmt *ASTImporter::Import(Stmt *FromS) {
   if (!FromS)
 return nullptr;
@@ -7845,6 +7879,12 @@
   return *ToSOrErr;
 }
 
+Expected ASTImporter::Import_New(NestedNameSpecifier *FromNNS) {
+  NestedNameSpecifier *ToNNS = Import(FromNNS);
+  if (!ToNNS && FromNNS)
+return llvm::make_error();
+  return ToNNS;
+}
 NestedNameSpecifier *ASTImporter::Import(NestedNameSpecifier *FromNNS) {
   if (!FromNNS)
 return nullptr;
@@ -7898,6 +7938,10 @@
   llvm_unreachable("Invalid nested name specifier kind");
 }
 
+Expected ASTImporter::Import_New(NestedNameSpecifierLoc FromNNS) {
+  NestedNameSpecifierLoc ToNNS = Import(FromNNS);
+  return ToNNS;
+}
 NestedNameSpecifierLoc ASTImporter::Import(NestedNameSpecifierLoc FromNNS) {
   // Copied from NestedNameSpecifier mostly.
   SmallVector NestedNames;
@@ -7969,6 +8013,12 @@
   return Builder.getWithLocInContext(getToContext());
 }
 
+Expected ASTImporter::Import_New(TemplateName From) {
+  TemplateName To = Import(From);
+  if (To.isNull() && !From.isNull())
+return llvm::make_error();
+  return To;
+}
 TemplateName ASTImporter::Import(TemplateName From) {
   switch (From.getKind()) {
   case TemplateName::Template:
@@ -8059,6 +8109,12 @@
   llvm_unreachable("Invalid template name kind");
 }
 
+Expected ASTImporter::Import_New(SourceLocation FromLoc) {
+  SourceLocation ToLoc = Import(FromLoc);
+  if (ToLoc.isInvalid() && !FromLoc.isInvalid())
+return llvm::make_error();
+  return ToLoc;
+}
 SourceLocation ASTImporter::Import(SourceLocation FromLoc) {
   if (FromLoc.isInvalid())
 return {};
@@ -8073,10 +8129,20 @@
   return ToSM.getComposedLoc(ToFileID, Decomposed.second);
 }
 
+Expected ASTImporter::Import_New(SourceRange FromRange) {
+  SourceRange ToRange = Import(FromRange);
+  return ToRange;
+}
 SourceRange ASTImporter::Import(SourceRange FromRange) {
   return SourceRange(Import(FromRange.getBegin()), Import(FromRange.getEnd()));
 }
 
+Expected ASTImporter::Import_New(FileID FromID) {
+  FileID ToID = Import(FromID);
+  if (ToID.isInvalid() && FromID.isValid())
+return llvm::make_error();
+  return ToID;
+}
 FileID ASTImporter::Import(FileID FromID) {
   llvm::DenseMap::iterator Pos = ImportedFileIDs.find(FromID);
   if (Pos != ImportedFileIDs.end())
@@ -8134,6 +8200,12 @@
   return ToID;
 }
 
+Expected ASTImporter::Import_New(CXXCtorInitializer *From) {
+  CXXCtorInitializer *To = Import(From);
+  if (!To && From)
+return llvm::make_error();
+  return To;
+}
 CXXCtorInitializer *ASTImporter::Import(CXXCtorInitializer *From) {
   Expr *ToExpr = Import(From->getInit());
   if (!ToExpr && From->getInit())
@@ -8179,6 +8251,12 @@
   }
 }
 
+Expected ASTImporter::Import_New(const CXXBaseSpecifier *From) {
+  CXXBaseSpecifier 

[PATCH] D53751: [ASTImporter] Added Import functions for transition to new API.

[Balázs Kéri via Phabricator via cfe-commits]

balazske updated this revision to Diff 171278.
balazske added a comment.

- Corrected Import_New(const Attr *)


Repository:
  rC Clang

https://reviews.llvm.org/D53751

Files:
  include/clang/AST/ASTImporter.h
  lib/AST/ASTImporter.cpp

Index: lib/AST/ASTImporter.cpp
===
--- lib/AST/ASTImporter.cpp
+++ lib/AST/ASTImporter.cpp
@@ -7656,6 +7656,12 @@
 
 ASTImporter::~ASTImporter() = default;
 
+Expected ASTImporter::Import_New(QualType FromT) {
+  QualType ToT = Import(FromT);
+  if (ToT.isNull() && !FromT.isNull())
+return make_error();
+  return ToT;
+}
 QualType ASTImporter::Import(QualType FromT) {
   if (FromT.isNull())
 return {};
@@ -7682,6 +7688,12 @@
   return ToContext.getQualifiedType(*ToTOrErr, FromT.getLocalQualifiers());
 }
 
+Expected ASTImporter::Import_New(TypeSourceInfo *FromTSI) {
+  TypeSourceInfo *ToTSI = Import(FromTSI);
+  if (!ToTSI && FromTSI)
+return llvm::make_error();
+  return ToTSI;
+}
 TypeSourceInfo *ASTImporter::Import(TypeSourceInfo *FromTSI) {
   if (!FromTSI)
 return FromTSI;
@@ -7696,8 +7708,12 @@
   T, Import(FromTSI->getTypeLoc().getBeginLoc()));
 }
 
+Expected ASTImporter::Import_New(const Attr *FromAttr) {
+  return Import(FromAttr);
+}
 Attr *ASTImporter::Import(const Attr *FromAttr) {
   Attr *ToAttr = FromAttr->clone(ToContext);
+  // NOTE: Import of SourceRange may fail.
   ToAttr->setRange(Import(FromAttr->getRange()));
   return ToAttr;
 }
@@ -7715,6 +7731,12 @@
   }
 }
 
+Expected ASTImporter::Import_New(Decl *FromD) {
+  Decl *ToD = Import(FromD);
+  if (!ToD && FromD)
+return llvm::make_error();
+  return ToD;
+}
 Decl *ASTImporter::Import(Decl *FromD) {
   if (!FromD)
 return nullptr;
@@ -7803,13 +7825,25 @@
   return ToDC;
 }
 
+Expected ASTImporter::Import_New(Expr *FromE) {
+  Expr *ToE = Import(FromE);
+  if (!ToE && FromE)
+return llvm::make_error();
+  return ToE;
+}
 Expr *ASTImporter::Import(Expr *FromE) {
   if (!FromE)
 return nullptr;
 
   return cast_or_null(Import(cast(FromE)));
 }
 
+Expected ASTImporter::Import_New(Stmt *FromS) {
+  Stmt *ToS = Import(FromS);
+  if (!ToS && FromS)
+return llvm::make_error();
+  return ToS;
+}
 Stmt *ASTImporter::Import(Stmt *FromS) {
   if (!FromS)
 return nullptr;
@@ -7845,6 +7879,12 @@
   return *ToSOrErr;
 }
 
+Expected ASTImporter::Import_New(NestedNameSpecifier *FromNNS) {
+  NestedNameSpecifier *ToNNS = Import(FromNNS);
+  if (!ToNNS && FromNNS)
+return llvm::make_error();
+  return ToNNS;
+}
 NestedNameSpecifier *ASTImporter::Import(NestedNameSpecifier *FromNNS) {
   if (!FromNNS)
 return nullptr;
@@ -7898,6 +7938,10 @@
   llvm_unreachable("Invalid nested name specifier kind");
 }
 
+Expected ASTImporter::Import_New(NestedNameSpecifierLoc FromNNS) {
+  NestedNameSpecifierLoc ToNNS = Import(FromNNS);
+  return ToNNS;
+}
 NestedNameSpecifierLoc ASTImporter::Import(NestedNameSpecifierLoc FromNNS) {
   // Copied from NestedNameSpecifier mostly.
   SmallVector NestedNames;
@@ -7969,6 +8013,12 @@
   return Builder.getWithLocInContext(getToContext());
 }
 
+Expected ASTImporter::Import_New(TemplateName From) {
+  TemplateName To = Import(From);
+  if (To.isNull() && !From.isNull())
+return llvm::make_error();
+  return To;
+}
 TemplateName ASTImporter::Import(TemplateName From) {
   switch (From.getKind()) {
   case TemplateName::Template:
@@ -8059,6 +8109,12 @@
   llvm_unreachable("Invalid template name kind");
 }
 
+Expected ASTImporter::Import_New(SourceLocation FromLoc) {
+  SourceLocation ToLoc = Import(FromLoc);
+  if (ToLoc.isInvalid() && !FromLoc.isInvalid())
+return llvm::make_error();
+  return ToLoc;
+}
 SourceLocation ASTImporter::Import(SourceLocation FromLoc) {
   if (FromLoc.isInvalid())
 return {};
@@ -8073,10 +8129,20 @@
   return ToSM.getComposedLoc(ToFileID, Decomposed.second);
 }
 
+Expected ASTImporter::Import_New(SourceRange FromRange) {
+  SourceRange ToRange = Import(FromRange);
+  return ToRange;
+}
 SourceRange ASTImporter::Import(SourceRange FromRange) {
   return SourceRange(Import(FromRange.getBegin()), Import(FromRange.getEnd()));
 }
 
+Expected ASTImporter::Import_New(FileID FromID) {
+  FileID ToID = Import(FromID);
+  if (ToID.isInvalid() && FromID.isValid())
+return llvm::make_error();
+  return ToID;
+}
 FileID ASTImporter::Import(FileID FromID) {
   llvm::DenseMap::iterator Pos = ImportedFileIDs.find(FromID);
   if (Pos != ImportedFileIDs.end())
@@ -8134,6 +8200,12 @@
   return ToID;
 }
 
+Expected ASTImporter::Import_New(CXXCtorInitializer *From) {
+  CXXCtorInitializer *To = Import(From);
+  if (!To && From)
+return llvm::make_error();
+  return To;
+}
 CXXCtorInitializer *ASTImporter::Import(CXXCtorInitializer *From) {
   Expr *ToExpr = Import(From->getInit());
   if (!ToExpr && From->getInit())
@@ -8179,6 +8251,12 @@
   }
 }
 
+Expected ASTImporter::Import_New(const CXXBaseSpecifier *From) {
+  CXXBaseSpecifier *To = Import(From);
+  

[PATCH] D53751: [ASTImporter] Added Import functions for transition to new API.

[Balázs Kéri via Phabricator via cfe-commits]

balazske created this revision.
Herald added subscribers: cfe-commits, Szelethus, martong, dkrupp.
Herald added a reviewer: a.sidorin.

These Import_New functions should be used in the ASTImporter,
and the old Import functions should not be used. Later the
Import_New should be renamed to Import again and the old Import
functions must be removed. But this can happen only after LLDB
was updated to use the new Import interface.

This commit is only about introducing the new Import_New
functions. These are not implemented now, only calling the old
Import ones.


Repository:
  rC Clang

https://reviews.llvm.org/D53751

Files:
  include/clang/AST/ASTImporter.h
  lib/AST/ASTImporter.cpp

Index: lib/AST/ASTImporter.cpp
===
--- lib/AST/ASTImporter.cpp
+++ lib/AST/ASTImporter.cpp
@@ -7656,6 +7656,12 @@
 
 ASTImporter::~ASTImporter() = default;
 
+Expected ASTImporter::Import_New(QualType FromT) {
+  QualType ToT = Import(FromT);
+  if (ToT.isNull() && !FromT.isNull())
+return make_error();
+  return ToT;
+}
 QualType ASTImporter::Import(QualType FromT) {
   if (FromT.isNull())
 return {};
@@ -7682,6 +7688,12 @@
   return ToContext.getQualifiedType(*ToTOrErr, FromT.getLocalQualifiers());
 }
 
+Expected ASTImporter::Import_New(TypeSourceInfo *FromTSI) {
+  TypeSourceInfo *ToTSI = Import(FromTSI);
+  if (!ToTSI && FromTSI)
+return llvm::make_error();
+  return ToTSI;
+}
 TypeSourceInfo *ASTImporter::Import(TypeSourceInfo *FromTSI) {
   if (!FromTSI)
 return FromTSI;
@@ -7715,6 +7727,12 @@
   }
 }
 
+Expected ASTImporter::Import_New(Decl *FromD) {
+  Decl *ToD = Import(FromD);
+  if (!ToD && FromD)
+return llvm::make_error();
+  return ToD;
+}
 Decl *ASTImporter::Import(Decl *FromD) {
   if (!FromD)
 return nullptr;
@@ -7803,13 +7821,25 @@
   return ToDC;
 }
 
+Expected ASTImporter::Import_New(Expr *FromE) {
+  Expr *ToE = Import(FromE);
+  if (!ToE && FromE)
+return llvm::make_error();
+  return ToE;
+}
 Expr *ASTImporter::Import(Expr *FromE) {
   if (!FromE)
 return nullptr;
 
   return cast_or_null(Import(cast(FromE)));
 }
 
+Expected ASTImporter::Import_New(Stmt *FromS) {
+  Stmt *ToS = Import(FromS);
+  if (!ToS && FromS)
+return llvm::make_error();
+  return ToS;
+}
 Stmt *ASTImporter::Import(Stmt *FromS) {
   if (!FromS)
 return nullptr;
@@ -7845,6 +7875,12 @@
   return *ToSOrErr;
 }
 
+Expected ASTImporter::Import_New(NestedNameSpecifier *FromNNS) {
+  NestedNameSpecifier *ToNNS = Import(FromNNS);
+  if (!ToNNS && FromNNS)
+return llvm::make_error();
+  return ToNNS;
+}
 NestedNameSpecifier *ASTImporter::Import(NestedNameSpecifier *FromNNS) {
   if (!FromNNS)
 return nullptr;
@@ -7898,6 +7934,10 @@
   llvm_unreachable("Invalid nested name specifier kind");
 }
 
+Expected ASTImporter::Import_New(NestedNameSpecifierLoc FromNNS) {
+  NestedNameSpecifierLoc ToNNS = Import(FromNNS);
+  return ToNNS;
+}
 NestedNameSpecifierLoc ASTImporter::Import(NestedNameSpecifierLoc FromNNS) {
   // Copied from NestedNameSpecifier mostly.
   SmallVector NestedNames;
@@ -7969,6 +8009,12 @@
   return Builder.getWithLocInContext(getToContext());
 }
 
+Expected ASTImporter::Import_New(TemplateName From) {
+  TemplateName To = Import(From);
+  if (To.isNull() && !From.isNull())
+return llvm::make_error();
+  return To;
+}
 TemplateName ASTImporter::Import(TemplateName From) {
   switch (From.getKind()) {
   case TemplateName::Template:
@@ -8059,6 +8105,12 @@
   llvm_unreachable("Invalid template name kind");
 }
 
+Expected ASTImporter::Import_New(SourceLocation FromLoc) {
+  SourceLocation ToLoc = Import(FromLoc);
+  if (ToLoc.isInvalid() && !FromLoc.isInvalid())
+return llvm::make_error();
+  return ToLoc;
+}
 SourceLocation ASTImporter::Import(SourceLocation FromLoc) {
   if (FromLoc.isInvalid())
 return {};
@@ -8073,10 +8125,20 @@
   return ToSM.getComposedLoc(ToFileID, Decomposed.second);
 }
 
+Expected ASTImporter::Import_New(SourceRange FromRange) {
+  SourceRange ToRange = Import(FromRange);
+  return ToRange;
+}
 SourceRange ASTImporter::Import(SourceRange FromRange) {
   return SourceRange(Import(FromRange.getBegin()), Import(FromRange.getEnd()));
 }
 
+Expected ASTImporter::Import_New(FileID FromID) {
+  FileID ToID = Import(FromID);
+  if (ToID.isInvalid() && FromID.isValid())
+return llvm::make_error();
+  return ToID;
+}
 FileID ASTImporter::Import(FileID FromID) {
   llvm::DenseMap::iterator Pos = ImportedFileIDs.find(FromID);
   if (Pos != ImportedFileIDs.end())
@@ -8134,6 +8196,12 @@
   return ToID;
 }
 
+Expected ASTImporter::Import_New(CXXCtorInitializer *From) {
+  CXXCtorInitializer *To = Import(From);
+  if (!To && From)
+return llvm::make_error();
+  return To;
+}
 CXXCtorInitializer *ASTImporter::Import(CXXCtorInitializer *From) {
   Expr *ToExpr = Import(From->getInit());
   if (!ToExpr && From->getInit())
@@ -8179,6 +8247,12 @@
   }
 }
 
+Expected ASTImporter::Import_New(const