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.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D61438/new/
https://reviews.llvm.org/D61438
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
