[PATCH] D152096: [Clang] Check for abstract parameters only when functions are defined.

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: clang/lib/Sema/SemaDeclCXX.cpp:6007
+  // non-abstract types.
+  if (!FD->hasSkippedBody())
     return;
----------------
This doesn't seem quite right -- we skip bodies for various reasons: https://github.com/llvm/llvm-project/blob/main/clang/include/clang/Parse/Parser.h#L413

Which test case needed this change?


================
Comment at: clang/test/CXX/class.derived/class.abstract/p3.cpp:38-41
+void f(A){}; // expected-error {{abstract class}}
+void f(A[1]){}; // expected-error {{abstract class}}
+void f(B){}; // expected-error {{abstract class}}
+void f(B[1]){}; // expected-error {{abstract class}}
----------------
Whole lotta trailing semi colons that should be removed (you should get all of them, not just theses).


================
Comment at: clang/test/CXX/class.derived/class.abstract/p3.cpp:77-79
   // FIXME: These should be handled consistently. We currently reject the first
   // two early because we (probably incorrectly, depending on dr1640) take
   // abstractness into account in forming implicit conversion sequences.
----------------
FIXME can be removed now? Do we need a DR test case for this?


================
Comment at: clang/test/CXX/drs/dr6xx.cpp:707-708
 
   // FIXME: The following examples demonstrate that we might be accepting the
   // above cases for the wrong reason.
 
----------------
Is this fixme still accurate? Should we be changing the resolution from partial to full?


================
Comment at: clang/test/SemaCXX/abstract.cpp:369
+foo  g();
+}
----------------
A mildly horrifying test case to consider:
```
struct S {
  virtual void func() = 0;
};
void S::func() {}

static_assert(__is_abstract(S));

struct T {
  void func(S) = delete;
  void other(S);
  void yet_another(S) {}
};
```
We currently don't do good things... https://godbolt.org/z/7vEY3Kz9c


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152096