[Lldb-commits] [PATCH] D61438: [ASTImporter] Use llvm::Expected and Error in the importer API

[Gabor Marton via Phabricator via lldb-commits]

martong marked an inline comment as done.
martong added inline comments.


================
Comment at: clang/include/clang/AST/ASTImporter.h:203
     /// context, or the import error.
-    llvm::Expected<TypeSourceInfo *> Import_New(TypeSourceInfo *FromTSI);
-    // FIXME: Remove this version.
-    TypeSourceInfo *Import(TypeSourceInfo *FromTSI);
+    llvm::Expected<TypeSourceInfo *> Import(TypeSourceInfo *FromTSI);
 
----------------
aprantl wrote:
> martong wrote:
> > aprantl wrote:
> > > Wouldn't it make more sense to return `Expected<TypeSourceInfo &>`?
> > That would not be consistent with the other changes. In this patch we change the signature of each functions similarly:
> > From `T foo(...)` to `Expected<T> foo(...)`.
> > Also, `TypeSourceInfo` factory functions in `ASTContext` do return with a pointer. For example:
> > ```
> >   TypeSourceInfo *CreateTypeSourceInfo(QualType T, unsigned Size = 0) const;
> > 
> >   /// Allocate a TypeSourceInfo where all locations have been
> >   /// initialized to a given location, which defaults to the empty
> >   /// location.
> >   TypeSourceInfo *
> >   getTrivialTypeSourceInfo(QualType T,
> >                            SourceLocation Loc = SourceLocation()) const;
> > 
> > ```
> I believe that LLVM's practice of passing non-null pointers around is bad API design because it's never quite clear from looking at an API which pointer parameters are nullable, so I was hoping that we could correct some of that at least in the cases where we introduce API that return Expected<> objects to make it obvious that this is either an error or a valid object. If you that the perceived inconsistency weighs stronger than the readability improvements let me know and I can reconsider.
I couldn't agree more that having non-null-able pointers is a bad API, and we'd be better to have references in these cases. However, I think the situation is more complex than that.

If we changed `TypeSourceInfo *` to `TypeSourceInfo &` in this patch, that would involve to make similar changes with other importer functions (NestedNameSpecifier *, Decl *, Expr *, etc.). That would result in a **huge** patch and I am afraid we could not follow the incremental development suggestion in the LLVM dev policy. Thus, even if we decide to change to references I propose to do that in a separate patch.

Also, some AST nodes are indeed null-able. E.g. the body of a FunctionDecl might be null. Or the described class template of a CXXRecordDecl may be null (and we rely on this, because first we create the CXXRecordDecl then we import the ClassTemplateDecl and only then we set the relation).
The point is that we should identify those AST nodes which are really non-nullable in the AST. This does not seem trivial.
Besides, since the ASTImporter class is part of the AST lib, it would look strange if we changed to use references only in the ASTImporter. Perhaps, we should change in the whole AST API, but that might broke too many dependencies I guess. Still, this may worth an RFC on cfe-dev.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D61438