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

Adrian Prantl via Phabricator via lldb-commits lldb-commits at lists.llvm.org
Mon May 13 11:41:47 PDT 2019

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

  rG LLVM Github Monorepo



More information about the lldb-commits mailing list