[PATCH] D44480: [Sema] Don't skip function bodies with 'auto' without trailing return type

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

In https://reviews.llvm.org/D44480#1117230, @nik wrote:

> In https://reviews.llvm.org/D44480#1117147, @cpplearner wrote:
>
> > Does `getAs()` work correctly with function returning `auto&`?
>
>
> the "getAs()" version will skip the function body and generate an 
> error message on use, but "FD->getReturnType()->getContainedDeducedType()" 
> works fine (will not skip the body, as it should here). OK, so now I have a 
> rough idea what the "Contained" was referring too :)


Yep, `getContainedDeducedType()` call is definitely necessary. Added the tests 
for discussed cases in https://reviews.llvm.org/rL333735.


Repository:
  rL LLVM

https://reviews.llvm.org/D44480



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

[PATCH] D44480: [Sema] Don't skip function bodies with 'auto' without trailing return type

[Nikolai Kosjar via Phabricator via cfe-commits]

nik added a comment.

In https://reviews.llvm.org/D44480#1117147, @cpplearner wrote:

> Does `getAs()` work correctly with function returning `auto&`?


No.  For

  class Foo {} foo;
  auto& return_auto_ref() { return foo; }
  auto r6 = return_auto_ref();

the "getAs()" version will skip the function body and generate an 
error message on use, but "FD->getReturnType()->getContainedDeducedType()" 
works fine (will not skip the body, as it should here). OK, so now I have a 
rough idea what the "Contained" was referring too :)


Repository:
  rL LLVM

https://reviews.llvm.org/D44480



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

[PATCH] D44480: [Sema] Don't skip function bodies with 'auto' without trailing return type

[S. B. Tam via Phabricator via cfe-commits]

cpplearner added a comment.

Does `getAs()` work correctly with function returning `auto&`?


Repository:
  rL LLVM

https://reviews.llvm.org/D44480



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

[PATCH] D44480: [Sema] Don't skip function bodies with 'auto' without trailing return type

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

In https://reviews.llvm.org/D44480#1116359, @nik wrote:

> I've stumbled about this bug too and was looking into it and then I saw the 
> mail about this change being submitted :)
>
> I've ended up with a slightly different change (https://dpaste.de/PSpM/raw , 
> instead of FD->getReturnType()->getContainedDeducedType() I have 
> FD->getReturnType()->getAs()). It looks like the tests from this 
> change pass with my change and my tests pass with this change (consider to 
> add decltype(auto) tests, too!). So I'm wondering whether there is a test 
> case that fails for one change but not the other (if there is, the test case 
> should probably be added), or if there are differences in the performance.
>
> I'm not familiar enough with the code base to justify the getAs(), 
> I've just observed that it made my tests pass.


I'm sure there should be no significant differences in performance. Adding 
`decltype(auto)`  to the test would certainly be useful.
W.r.t. correctness I think at this point checks for DeducedType and AutoType 
are equivalent. There is only one other inheritor of DeducedType, the 
DeducedTemplateSpecializationType, and it should not appear in the function 
return type IIUC.

When the concepts get into the standard and clangd implements them, this might 
change, as the comment of `DeducedType` suggests:

  /// Common base class for placeholders for types that get replaced by
  /// placeholder type deduction: C++11 auto, C++14 decltype(auto), C++17 
deduced
  /// class template types, and (eventually) constrained type names from the C++
  /// Concepts TS.

I don't know the exact details of the latest Concepts proposal, but would 
assume usages of constrained template parameters from Concepts TS shouldn't 
block skipping of function bodies. In which case, your fix seems to do the 
right thing and this one doesn't.
@rsmith, WDYT? Should we check for AutoType instead?


Repository:
  rL LLVM

https://reviews.llvm.org/D44480



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

[PATCH] D44480: [Sema] Don't skip function bodies with 'auto' without trailing return type

[Nikolai Kosjar via Phabricator via cfe-commits]

nik added a comment.

I've stumbled about this bug too and was looking into it and then I saw the 
mail about this change being submitted :)

I've ended up with a slightly different change (https://dpaste.de/PSpM/raw , 
instead of FD->getReturnType()->getContainedDeducedType() I have 
FD->getReturnType()->getAs()). It looks like the tests from this 
change pass with my change and my tests pass with this change (consider to add 
decltype(auto) tests, too!). So I'm wondering whether there is a test case that 
fails for one change but not the other (if there is, the test case should 
probably be added), or if there are differences in the performance.

I'm not familiar enough with the code base to justify the getAs(), 
I've just observed that it made my tests pass.


Repository:
  rL LLVM

https://reviews.llvm.org/D44480



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

[PATCH] D44480: [Sema] Don't skip function bodies with 'auto' without trailing return type

[Phabricator via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL333538: [Sema] Dont skip function bodies with 
auto without trailing return type (authored by ibiryukov, committed 
by ).
Herald added a subscriber: llvm-commits.

Repository:
  rL LLVM

https://reviews.llvm.org/D44480

Files:
  cfe/trunk/lib/Sema/SemaDecl.cpp
  cfe/trunk/test/CodeCompletion/crash-skipped-bodies-template-inst.cpp
  cfe/trunk/test/CodeCompletion/skip-auto-funcs.cpp

Index: cfe/trunk/lib/Sema/SemaDecl.cpp
===
--- cfe/trunk/lib/Sema/SemaDecl.cpp
+++ cfe/trunk/lib/Sema/SemaDecl.cpp
@@ -12660,9 +12660,15 @@
   // rest of the file.
   // We cannot skip the body of a function with an undeduced return type,
   // because any callers of that function need to know the type.
-  if (const FunctionDecl *FD = D->getAsFunction())
-if (FD->isConstexpr() || FD->getReturnType()->isUndeducedType())
+  if (const FunctionDecl *FD = D->getAsFunction()) {
+if (FD->isConstexpr())
   return false;
+// We can't simply call Type::isUndeducedType here, because inside template
+// auto can be deduced to a dependent type, which is not considered
+// "undeduced".
+if (FD->getReturnType()->getContainedDeducedType())
+  return false;
+  }
   return Consumer.shouldSkipFunctionBody(D);
 }
 
Index: cfe/trunk/test/CodeCompletion/skip-auto-funcs.cpp
===
--- cfe/trunk/test/CodeCompletion/skip-auto-funcs.cpp
+++ cfe/trunk/test/CodeCompletion/skip-auto-funcs.cpp
@@ -0,0 +1,43 @@
+// We run clang in completion mode to force skipping of function bodies and
+// check if the function bodies were skipped by observing the warnings that
+// clang produces.
+// RUN: not %clang_cc1 -fsyntax-only -code-completion-at=%s:42:1 %s -o - 2>&1 | FileCheck %s
+template 
+auto not_skipped() {
+  int x;
+  if (x = 10) {}
+  // Check that this function is not skipped.
+  // CHECK: 8:9: warning: using the result of an assignment as a condition without parentheses
+  return 1;
+}
+
+template 
+auto lambda_not_skipped = []() {
+  int x;
+  if (x = 10) {}
+  // Check that this function is not skipped.
+  // CHECK: 17:9: warning: using the result of an assignment as a condition without parentheses
+  return 1;
+}
+
+template 
+auto skipped() -> T {
+  int x;
+  if (x = 10) {}
+  // Check that this function is skipped.
+  // CHECK-NOT: 26:9: warning: using the result of an assignment as a condition without parentheses
+  return 1;
+};
+
+auto lambda_skipped = []() -> int {
+  int x;
+  if (x = 10) {}
+  // This could potentially be skipped, but it isn't at the moment.
+  // CHECK: 34:9: warning: using the result of an assignment as a condition without parentheses
+  return 1;
+};
+
+int test() {
+  int complete_in_this_function;
+  // CHECK: COMPLETION: complete_in_this_function
+}
Index: cfe/trunk/test/CodeCompletion/crash-skipped-bodies-template-inst.cpp
===
--- cfe/trunk/test/CodeCompletion/crash-skipped-bodies-template-inst.cpp
+++ cfe/trunk/test/CodeCompletion/crash-skipped-bodies-template-inst.cpp
@@ -0,0 +1,27 @@
+// RUN: %clang_cc1 -fsyntax-only -code-completion-at=%s:24:5 %s -o - 2>&1 | FileCheck %s
+template 
+auto make_func() {
+  struct impl {
+impl* func() {
+  int x;
+  if (x = 10) {}
+  // Check that body of this function is actually skipped.
+  // CHECK-NOT: crash-skipped-bodies-template-inst.cpp:7:{{[0-9]+}}: warning: using the result of an assignment as a condition without parentheses
+  return this;
+}
+  };
+
+  int x;
+  if (x = 10) {}
+  // Check that this function is not skipped.
+  // CHECK: crash-skipped-bodies-template-inst.cpp:15:9: warning: using the result of an assignment as a condition without parentheses
+  return impl();
+}
+
+void foo() {
+  []() {
+make_func();
+m
+// CHECK: COMPLETION: make_func : [#auto#]make_func<<#class T#>>()
+  };
+}
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D44480: [Sema] Don't skip function bodies with 'auto' without trailing return type

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov updated this revision to Diff 149099.
ilya-biryukov added a comment.

- Fix the comments


Repository:
  rC Clang

https://reviews.llvm.org/D44480

Files:
  lib/Sema/SemaDecl.cpp
  test/CodeCompletion/crash-skipped-bodies-template-inst.cpp
  test/CodeCompletion/skip-auto-funcs.cpp

Index: test/CodeCompletion/skip-auto-funcs.cpp
===
--- /dev/null
+++ test/CodeCompletion/skip-auto-funcs.cpp
@@ -0,0 +1,43 @@
+// We run clang in completion mode to force skipping of function bodies and
+// check if the function bodies were skipped by observing the warnings that
+// clang produces.
+// RUN: not %clang_cc1 -fsyntax-only -code-completion-at=%s:42:1 %s -o - 2>&1 | FileCheck %s
+template 
+auto not_skipped() {
+  int x;
+  if (x = 10) {}
+  // Check that this function is not skipped.
+  // CHECK: 8:9: warning: using the result of an assignment as a condition without parentheses
+  return 1;
+}
+
+template 
+auto lambda_not_skipped = []() {
+  int x;
+  if (x = 10) {}
+  // Check that this function is not skipped.
+  // CHECK: 17:9: warning: using the result of an assignment as a condition without parentheses
+  return 1;
+}
+
+template 
+auto skipped() -> T {
+  int x;
+  if (x = 10) {}
+  // Check that this function is skipped.
+  // CHECK-NOT: 26:9: warning: using the result of an assignment as a condition without parentheses
+  return 1;
+};
+
+auto lambda_skipped = []() -> int {
+  int x;
+  if (x = 10) {}
+  // This could potentially be skipped, but it isn't at the moment.
+  // CHECK: 34:9: warning: using the result of an assignment as a condition without parentheses
+  return 1;
+};
+
+int test() {
+  int complete_in_this_function;
+  // CHECK: COMPLETION: complete_in_this_function
+}
Index: test/CodeCompletion/crash-skipped-bodies-template-inst.cpp
===
--- /dev/null
+++ test/CodeCompletion/crash-skipped-bodies-template-inst.cpp
@@ -0,0 +1,27 @@
+// RUN: %clang_cc1 -fsyntax-only -code-completion-at=%s:24:5 %s -o - 2>&1 | FileCheck %s
+template 
+auto make_func() {
+  struct impl {
+impl* func() {
+  int x;
+  if (x = 10) {}
+  // Check that body of this function is actually skipped.
+  // CHECK-NOT: crash-skipped-bodies-template-inst.cpp:7:{{[0-9]+}}: warning: using the result of an assignment as a condition without parentheses
+  return this;
+}
+  };
+
+  int x;
+  if (x = 10) {}
+  // Check that this function is not skipped.
+  // CHECK: crash-skipped-bodies-template-inst.cpp:15:9: warning: using the result of an assignment as a condition without parentheses
+  return impl();
+}
+
+void foo() {
+  []() {
+make_func();
+m
+// CHECK: COMPLETION: make_func : [#auto#]make_func<<#class T#>>()
+  };
+}
Index: lib/Sema/SemaDecl.cpp
===
--- lib/Sema/SemaDecl.cpp
+++ lib/Sema/SemaDecl.cpp
@@ -12660,9 +12660,15 @@
   // rest of the file.
   // We cannot skip the body of a function with an undeduced return type,
   // because any callers of that function need to know the type.
-  if (const FunctionDecl *FD = D->getAsFunction())
-if (FD->isConstexpr() || FD->getReturnType()->isUndeducedType())
+  if (const FunctionDecl *FD = D->getAsFunction()) {
+if (FD->isConstexpr())
   return false;
+// We can't simply call Type::isUndeducedType here, because inside template
+// auto can be deduced to a dependent type, which is not considered
+// "undeduced".
+if (FD->getReturnType()->getContainedDeducedType())
+  return false;
+  }
   return Consumer.shouldSkipFunctionBody(D);
 }
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D44480: [Sema] Don't skip function bodies with 'auto' without trailing return type

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith accepted this revision.
rsmith added inline comments.
This revision is now accepted and ready to land.



Comment at: test/CodeCompletion/skip-auto-funcs.cpp:9
+  if (x = 10) {}
+  // Check that this function is skipped.
+  // CHECK: 8:9: warning: using the result of an assignment as a condition 
without parentheses

is skipped -> is not skipped?



Comment at: test/CodeCompletion/skip-auto-funcs.cpp:18
+  if (x = 10) {}
+  // Check that this function is skipped.
+  // CHECK: 17:9: warning: using the result of an assignment as a condition 
without parentheses

is skipped -> is not skipped?


Repository:
  rC Clang

https://reviews.llvm.org/D44480



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

[PATCH] D44480: [Sema] Don't skip function bodies with 'auto' without trailing return type

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

Sorry for delays with this patch, I somehow missed the email notification about 
it.

> Expounding on my concerns a bit -- I'm worried about all the other places 
> calling isUndeducedType() and whether they're also at risk and thus a better 
> fix is to change isUndeducedType() to pay attention to the language mode.

I thought semantics of `isUndeducedType()` is actually what's most callers 
(e.g. diagnostics) want. That is "the type is undeduced **and** not dependent". 
But the name can definitely bring some confusion, so there might be other 
places that misinterpreted it.

@rsmith, it would be great if you could take another look when you have time.




Comment at: lib/Sema/SemaDecl.cpp:12609-12610
   return false;
+// We can't simply call Type::isUndeducedType here, because it returns
+// false on C++14's auto return type without trailing return type.
+DeducedType *DT = FD->getReturnType()->getContainedDeducedType();

rsmith wrote:
> I don't think this comment captures the problem. Rather, I think the problem 
> is that in a template we deduce the `auto` to a dependent type, so it's not 
> considered "undeduced" at the point when we ask this question.
Thanks! That definitely explains the problem in a proper manner.



Comment at: lib/Sema/SemaDecl.cpp:12612-12613
+DeducedType *DT = FD->getReturnType()->getContainedDeducedType();
+if (DT && DT->getDeducedType().isNull())
+  return false;
+  }

rsmith wrote:
> Is there any need to check whether the type has been deduced here? It seems 
> simpler (and equivalent) to turn off skipping for functions whose return type 
> involves a deduced type regardless of whether deduction has been performed 
> already (which it hasn't).
Thanks! I misinterpreted the deduced type's semantics. I thought it also 
appears in the following case:
`auto foo() -> int {}`
But it doesn't.


Repository:
  rC Clang

https://reviews.llvm.org/D44480



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

[PATCH] D44480: [Sema] Don't skip function bodies with 'auto' without trailing return type

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov updated this revision to Diff 148835.
ilya-biryukov marked 2 inline comments as done.
ilya-biryukov added a comment.

- Address review comments.
- Added one more test.


Repository:
  rC Clang

https://reviews.llvm.org/D44480

Files:
  lib/Sema/SemaDecl.cpp
  test/CodeCompletion/crash-skipped-bodies-template-inst.cpp
  test/CodeCompletion/skip-auto-funcs.cpp

Index: test/CodeCompletion/skip-auto-funcs.cpp
===
--- /dev/null
+++ test/CodeCompletion/skip-auto-funcs.cpp
@@ -0,0 +1,43 @@
+// We run clang in completion mode to force skipping of function bodies and
+// check if the function bodies were skipped by observing the warnings that
+// clang produces.
+// RUN: not %clang_cc1 -fsyntax-only -code-completion-at=%s:42:1 %s -o - 2>&1 | FileCheck %s
+template 
+auto not_skipped() {
+  int x;
+  if (x = 10) {}
+  // Check that this function is skipped.
+  // CHECK: 8:9: warning: using the result of an assignment as a condition without parentheses
+  return 1;
+}
+
+template 
+auto lambda_not_skipped = []() {
+  int x;
+  if (x = 10) {}
+  // Check that this function is skipped.
+  // CHECK: 17:9: warning: using the result of an assignment as a condition without parentheses
+  return 1;
+}
+
+template 
+auto skipped() -> T {
+  int x;
+  if (x = 10) {}
+  // Check that this function is skipped.
+  // CHECK-NOT: 26:9: warning: using the result of an assignment as a condition without parentheses
+  return 1;
+};
+
+auto lambda_skipped = []() -> int {
+  int x;
+  if (x = 10) {}
+  // This could potentially be skipped, but it isn't at the moment.
+  // CHECK: 34:9: warning: using the result of an assignment as a condition without parentheses
+  return 1;
+};
+
+int test() {
+  int complete_in_this_function;
+  // CHECK: COMPLETION: complete_in_this_function
+}
Index: test/CodeCompletion/crash-skipped-bodies-template-inst.cpp
===
--- /dev/null
+++ test/CodeCompletion/crash-skipped-bodies-template-inst.cpp
@@ -0,0 +1,27 @@
+// RUN: %clang_cc1 -fsyntax-only -code-completion-at=%s:24:5 %s -o - 2>&1 | FileCheck %s
+template 
+auto make_func() {
+  struct impl {
+impl* func() {
+  int x;
+  if (x = 10) {}
+  // Check that body of this function is actually skipped.
+  // CHECK-NOT: crash-skipped-bodies-template-inst.cpp:7:{{[0-9]+}}: warning: using the result of an assignment as a condition without parentheses
+  return this;
+}
+  };
+
+  int x;
+  if (x = 10) {}
+  // Check that this function is not skipped.
+  // CHECK: crash-skipped-bodies-template-inst.cpp:15:9: warning: using the result of an assignment as a condition without parentheses
+  return impl();
+}
+
+void foo() {
+  []() {
+make_func();
+m
+// CHECK: COMPLETION: make_func : [#auto#]make_func<<#class T#>>()
+  };
+}
Index: lib/Sema/SemaDecl.cpp
===
--- lib/Sema/SemaDecl.cpp
+++ lib/Sema/SemaDecl.cpp
@@ -12658,9 +12658,15 @@
   // rest of the file.
   // We cannot skip the body of a function with an undeduced return type,
   // because any callers of that function need to know the type.
-  if (const FunctionDecl *FD = D->getAsFunction())
-if (FD->isConstexpr() || FD->getReturnType()->isUndeducedType())
+  if (const FunctionDecl *FD = D->getAsFunction()) {
+if (FD->isConstexpr())
   return false;
+// We can't simply call Type::isUndeducedType here, because inside template
+// auto can be deduced to a dependent type, which is not considered
+// "undeduced".
+if (FD->getReturnType()->getContainedDeducedType())
+  return false;
+  }
   return Consumer.shouldSkipFunctionBody(D);
 }
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D44480: [Sema] Don't skip function bodies with 'auto' without trailing return type

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added a comment.

Thanks, I think this can be simplified a bit but this looks like the right fix.




Comment at: lib/Sema/SemaDecl.cpp:12609-12610
   return false;
+// We can't simply call Type::isUndeducedType here, because it returns
+// false on C++14's auto return type without trailing return type.
+DeducedType *DT = FD->getReturnType()->getContainedDeducedType();

I don't think this comment captures the problem. Rather, I think the problem is 
that in a template we deduce the `auto` to a dependent type, so it's not 
considered "undeduced" at the point when we ask this question.



Comment at: lib/Sema/SemaDecl.cpp:12612-12613
+DeducedType *DT = FD->getReturnType()->getContainedDeducedType();
+if (DT && DT->getDeducedType().isNull())
+  return false;
+  }

Is there any need to check whether the type has been deduced here? It seems 
simpler (and equivalent) to turn off skipping for functions whose return type 
involves a deduced type regardless of whether deduction has been performed 
already (which it hasn't).


Repository:
  rC Clang

https://reviews.llvm.org/D44480



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

[PATCH] D44480: [Sema] Don't skip function bodies with 'auto' without trailing return type

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

Expounding on my concerns a bit -- I'm worried about all the other places 
calling `isUndeducedType()` and whether they're also at risk and thus a better 
fix is to change `isUndeducedType()` to pay attention to the language mode.




Comment at: lib/Sema/SemaDecl.cpp:12611
+// false on C++14's auto return type without trailing return type.
+DeducedType *DT = FD->getReturnType()->getContainedDeducedType();
+if (DT && DT->getDeducedType().isNull())

This can be marked `const`.


Repository:
  rC Clang

https://reviews.llvm.org/D44480



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

[PATCH] D44480: [Sema] Don't skip function bodies with 'auto' without trailing return type

[Raoul Wols via Phabricator via cfe-commits]

rwols added a comment.

Can we get this merged soon? I'm experiencing a lot of false positive 
diagnostics with this library  at work.


Repository:
  rC Clang

https://reviews.llvm.org/D44480



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

[PATCH] D44480: [Sema] Don't skip function bodies with 'auto' without trailing return type

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov marked an inline comment as done.
ilya-biryukov added a comment.

I hit that bug in practice a few days ago when playing around with a C++17 
codebase. It makes completion totally unusable when functions with auto-deduced 
return type are used.
@rsmith, any chance you could take a look? Or maybe suggest someone else as a 
reviewer?


Repository:
  rC Clang

https://reviews.llvm.org/D44480



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

[PATCH] D44480: [Sema] Don't skip function bodies with 'auto' without trailing return type

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov updated this revision to Diff 139926.
ilya-biryukov added a comment.

- Replace auto with explicit type


Repository:
  rC Clang

https://reviews.llvm.org/D44480

Files:
  lib/Sema/SemaDecl.cpp
  test/CodeCompletion/crash-skipped-bodies-template-inst.cpp


Index: test/CodeCompletion/crash-skipped-bodies-template-inst.cpp
===
--- /dev/null
+++ test/CodeCompletion/crash-skipped-bodies-template-inst.cpp
@@ -0,0 +1,27 @@
+// RUN: %clang_cc1 -fsyntax-only -code-completion-at=%s:24:5 %s -o - 2>&1 | 
FileCheck %s
+template 
+auto make_func() {
+  struct impl {
+impl* func() {
+  int x;
+  if (x = 10) {}
+  // Check that body of this function is actually skipped.
+  // CHECK-NOT: crash-skipped-bodies-template-inst.cpp:7:{{[0-9]+}}: 
warning: using the result of an assignment as a condition without parentheses
+  return this;
+}
+  };
+
+  int x;
+  if (x = 10) {}
+  // Check that this function is not skipped.
+  // CHECK: crash-skipped-bodies-template-inst.cpp:15:9: warning: using the 
result of an assignment as a condition without parentheses
+  return impl();
+}
+
+void foo() {
+  []() {
+make_func();
+m
+// CHECK: COMPLETION: make_func : [#auto#]make_func<<#class T#>>()
+  };
+}
Index: lib/Sema/SemaDecl.cpp
===
--- lib/Sema/SemaDecl.cpp
+++ lib/Sema/SemaDecl.cpp
@@ -12603,9 +12603,15 @@
   // rest of the file.
   // We cannot skip the body of a function with an undeduced return type,
   // because any callers of that function need to know the type.
-  if (const FunctionDecl *FD = D->getAsFunction())
-if (FD->isConstexpr() || FD->getReturnType()->isUndeducedType())
+  if (const FunctionDecl *FD = D->getAsFunction()) {
+if (FD->isConstexpr())
   return false;
+// We can't simply call Type::isUndeducedType here, because it returns
+// false on C++14's auto return type without trailing return type.
+DeducedType *DT = FD->getReturnType()->getContainedDeducedType();
+if (DT && DT->getDeducedType().isNull())
+  return false;
+  }
   return Consumer.shouldSkipFunctionBody(D);
 }
 


Index: test/CodeCompletion/crash-skipped-bodies-template-inst.cpp
===
--- /dev/null
+++ test/CodeCompletion/crash-skipped-bodies-template-inst.cpp
@@ -0,0 +1,27 @@
+// RUN: %clang_cc1 -fsyntax-only -code-completion-at=%s:24:5 %s -o - 2>&1 | FileCheck %s
+template 
+auto make_func() {
+  struct impl {
+impl* func() {
+  int x;
+  if (x = 10) {}
+  // Check that body of this function is actually skipped.
+  // CHECK-NOT: crash-skipped-bodies-template-inst.cpp:7:{{[0-9]+}}: warning: using the result of an assignment as a condition without parentheses
+  return this;
+}
+  };
+
+  int x;
+  if (x = 10) {}
+  // Check that this function is not skipped.
+  // CHECK: crash-skipped-bodies-template-inst.cpp:15:9: warning: using the result of an assignment as a condition without parentheses
+  return impl();
+}
+
+void foo() {
+  []() {
+make_func();
+m
+// CHECK: COMPLETION: make_func : [#auto#]make_func<<#class T#>>()
+  };
+}
Index: lib/Sema/SemaDecl.cpp
===
--- lib/Sema/SemaDecl.cpp
+++ lib/Sema/SemaDecl.cpp
@@ -12603,9 +12603,15 @@
   // rest of the file.
   // We cannot skip the body of a function with an undeduced return type,
   // because any callers of that function need to know the type.
-  if (const FunctionDecl *FD = D->getAsFunction())
-if (FD->isConstexpr() || FD->getReturnType()->isUndeducedType())
+  if (const FunctionDecl *FD = D->getAsFunction()) {
+if (FD->isConstexpr())
   return false;
+// We can't simply call Type::isUndeducedType here, because it returns
+// false on C++14's auto return type without trailing return type.
+DeducedType *DT = FD->getReturnType()->getContainedDeducedType();
+if (DT && DT->getDeducedType().isNull())
+  return false;
+  }
   return Consumer.shouldSkipFunctionBody(D);
 }
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D44480: [Sema] Don't skip function bodies with 'auto' without trailing return type

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a reviewer: rsmith.
aaron.ballman added a comment.

Adding Richard as a reviewer, because this seems somewhat clever to me (so 
there may be a better change to make).




Comment at: lib/Sema/SemaDecl.cpp:12611
+// false on C++14's auto return type without trailing return type.
+auto *DT = FD->getReturnType()->getContainedDeducedType();
+if (DT && DT->getDeducedType().isNull())

Please don't use `auto` here as the type is not spelled out explicitly. Also, 
the type can be const-qualified.


Repository:
  rC Clang

https://reviews.llvm.org/D44480



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

[PATCH] D44480: [Sema] Don't skip function bodies with 'auto' without trailing return type

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov created this revision.
ilya-biryukov added reviewers: aaron.ballman, sammccall.

Skipping them was clearly not intentional. It's impossible to
guarantee correctness if the bodies are skipped.
Also adds a test case for r327504, now that it does not produce
invalid errors that made the test fail.


Repository:
  rC Clang

https://reviews.llvm.org/D44480

Files:
  lib/Sema/SemaDecl.cpp
  test/CodeCompletion/crash-skipped-bodies-template-inst.cpp


Index: test/CodeCompletion/crash-skipped-bodies-template-inst.cpp
===
--- /dev/null
+++ test/CodeCompletion/crash-skipped-bodies-template-inst.cpp
@@ -0,0 +1,27 @@
+// RUN: %clang_cc1 -fsyntax-only -code-completion-at=%s:24:5 %s -o - 2>&1 | 
FileCheck %s
+template 
+auto make_func() {
+  struct impl {
+impl* func() {
+  int x;
+  if (x = 10) {}
+  // Check that body of this function is actually skipped.
+  // CHECK-NOT: crash-skipped-bodies-template-inst.cpp:7:{{[0-9]+}}: 
warning: using the result of an assignment as a condition without parentheses
+  return this;
+}
+  };
+
+  int x;
+  if (x = 10) {}
+  // Check that this function is not skipped.
+  // CHECK: crash-skipped-bodies-template-inst.cpp:15:9: warning: using the 
result of an assignment as a condition without parentheses
+  return impl();
+}
+
+void foo() {
+  []() {
+make_func();
+m
+// CHECK: COMPLETION: make_func : [#auto#]make_func<<#class T#>>()
+  };
+}
Index: lib/Sema/SemaDecl.cpp
===
--- lib/Sema/SemaDecl.cpp
+++ lib/Sema/SemaDecl.cpp
@@ -12603,9 +12603,15 @@
   // rest of the file.
   // We cannot skip the body of a function with an undeduced return type,
   // because any callers of that function need to know the type.
-  if (const FunctionDecl *FD = D->getAsFunction())
-if (FD->isConstexpr() || FD->getReturnType()->isUndeducedType())
+  if (const FunctionDecl *FD = D->getAsFunction()) {
+if (FD->isConstexpr())
   return false;
+// We can't simply call Type::isUndeducedType here, because it returns
+// false on C++14's auto return type without trailing return type.
+auto *DT = FD->getReturnType()->getContainedDeducedType();
+if (DT && DT->getDeducedType().isNull())
+  return false;
+  }
   return Consumer.shouldSkipFunctionBody(D);
 }
 


Index: test/CodeCompletion/crash-skipped-bodies-template-inst.cpp
===
--- /dev/null
+++ test/CodeCompletion/crash-skipped-bodies-template-inst.cpp
@@ -0,0 +1,27 @@
+// RUN: %clang_cc1 -fsyntax-only -code-completion-at=%s:24:5 %s -o - 2>&1 | FileCheck %s
+template 
+auto make_func() {
+  struct impl {
+impl* func() {
+  int x;
+  if (x = 10) {}
+  // Check that body of this function is actually skipped.
+  // CHECK-NOT: crash-skipped-bodies-template-inst.cpp:7:{{[0-9]+}}: warning: using the result of an assignment as a condition without parentheses
+  return this;
+}
+  };
+
+  int x;
+  if (x = 10) {}
+  // Check that this function is not skipped.
+  // CHECK: crash-skipped-bodies-template-inst.cpp:15:9: warning: using the result of an assignment as a condition without parentheses
+  return impl();
+}
+
+void foo() {
+  []() {
+make_func();
+m
+// CHECK: COMPLETION: make_func : [#auto#]make_func<<#class T#>>()
+  };
+}
Index: lib/Sema/SemaDecl.cpp
===
--- lib/Sema/SemaDecl.cpp
+++ lib/Sema/SemaDecl.cpp
@@ -12603,9 +12603,15 @@
   // rest of the file.
   // We cannot skip the body of a function with an undeduced return type,
   // because any callers of that function need to know the type.
-  if (const FunctionDecl *FD = D->getAsFunction())
-if (FD->isConstexpr() || FD->getReturnType()->isUndeducedType())
+  if (const FunctionDecl *FD = D->getAsFunction()) {
+if (FD->isConstexpr())
   return false;
+// We can't simply call Type::isUndeducedType here, because it returns
+// false on C++14's auto return type without trailing return type.
+auto *DT = FD->getReturnType()->getContainedDeducedType();
+if (DT && DT->getDeducedType().isNull())
+  return false;
+  }
   return Consumer.shouldSkipFunctionBody(D);
 }
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits