[PATCH] D52301: [clang] Set TypeSourceInfo for vardecl's in addition to type when we can deduce.

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

Thank you for the context and explanation! I don't feel like I have enough familiarity with this machinery to be an effective reviewer for signing off on anything. I've commented with some nits, but @rsmith should give the sign-off on this one.



================
Comment at: include/clang/AST/PrettyPrinter.h:229
+
+  /// When true, prints deduced type for auto typed variables if their type has
+  /// been deduced. When false, always prints auto(or other variants e.g.
----------------
prints deduced -> prints the deduced


================
Comment at: include/clang/AST/PrettyPrinter.h:230
+  /// When true, prints deduced type for auto typed variables if their type has
+  /// been deduced. When false, always prints auto(or other variants e.g.
+  /// decltype(auto)) even if type was deduced.
----------------
Add a whitespace before the left paren.


================
Comment at: include/clang/Sema/Sema.h:7063
 
-  QualType deduceVarTypeFromInitializer(VarDecl *VDecl, DeclarationName Name,
-                                        QualType Type, TypeSourceInfo *TSI,
-                                        SourceRange Range, bool DirectInit,
-                                        Expr *Init);
+  std::pair<QualType, TypeSourceInfo *>
+  deduceVarTypeFromInitializer(VarDecl *VDecl, DeclarationName Name,
----------------
I'd appreciate some comments as to why this is returning a pair and under what circumstances the type information will be different, because it's not likely to be obvious to anyone reading this header.


================
Comment at: lib/Frontend/ASTConsumers.cpp:92
         PrintingPolicy Policy(D->getASTContext().getLangOpts());
+        // Since ASTPrinter is used for pretty printing and auto is generally
+        // prettier than real type itself, we'll choose to print auto always.
----------------
Since -> Because

That said, I am not convinced I agree with this being the default. "Prettier" is pretty subjective, and I'd rather our default be for clarity instead of brevity, at least here.


================
Comment at: lib/Sema/SemaChecking.cpp:852
 
-  if (RT->getPointeeType().getAddressSpace() != LangAS::opencl_generic) {
-    S.Diag(Call->getArg(0)->getBeginLoc(),
----------------
kadircet wrote:
> aaron.ballman wrote:
> > I'm not certain I understand why this code has been removed?
> It shouldn't have been, tried rebasing but it didn't go away. I think it was deleted at some point by a different change and somehow ended up showing in here as well. (Tried to revert, got an error stating warn_opencl_generic_address_space_arg doesn't exist)
That's truly strange. I bring it up because these sort of changes make it harder for reviewers to test changes locally by applying the patch themselves.


================
Comment at: lib/Sema/SemaDecl.cpp:10912
                                          Expr *Init) {
-  QualType DeducedType = deduceVarTypeFromInitializer(
+  auto DeducedTypeAndTSI = deduceVarTypeFromInitializer(
       VDecl, VDecl->getDeclName(), VDecl->getType(), VDecl->getTypeSourceInfo(),
----------------
Don't use `auto` as the type is not spelled out in the initialization.


================
Comment at: lib/Sema/SemaStmt.cpp:1888
   if (First) {
-    QualType FirstType;
+    TypeSourceInfo* FirstTypeTSI;
     if (DeclStmt *DS = dyn_cast<DeclStmt>(First)) {
----------------
Formatting is off here (the `*` should bind to the right).


================
Comment at: lib/Sema/SemaStmt.cpp:1975
   // AddInitializerToDecl, so we can produce a more suitable diagnostic.
-  QualType InitType;
+  TypeSourceInfo* InitTypeTSI = nullptr;
   if ((!isa<InitListExpr>(Init) && Init->getType()->isVoidType()) ||
----------------
Formatting.


================
Comment at: lib/Sema/SemaStmt.cpp:3442
     //  argument deduction.
-    DeduceAutoResult DAR = DeduceAutoType(OrigResultType, RetExpr, Deduced);
+    TypeSourceInfo* DeducedTSI = nullptr;
+    DeduceAutoResult DAR = DeduceAutoType(OrigResultType, RetExpr, DeducedTSI);
----------------
Same -- you should probably run the patch through clang-format (https://clang.llvm.org/docs/ClangFormat.html#script-for-patch-reformatting).


================
Comment at: lib/Sema/SemaTemplateDeduction.cpp:4464
           .Apply(Type);
-  assert(!FuncParam.isNull() &&
+  assert(!FuncParamTSI->getType().isNull() &&
          "substituting template parameter for 'auto' failed");
----------------
Null check?


Repository:
  rC Clang

https://reviews.llvm.org/D52301