comex abandoned this revision.
comex marked 18 inline comments as done.
comex added inline comments.
================
Comment at: lib/Analysis/CFG.cpp:2102
case Stmt::CompoundStmtClass:
- return VisitCompoundStmt(cast<CompoundStmt>(S));
+ return VisitCompoundStmt(cast<CompoundStmt>(S),
/*ExternallyDestructed=*/false);
----------------
NoQ wrote:
> This goes over the 80 characters limit :)
Thanks, fixed.
================
Comment at: lib/Analysis/CFG.cpp:2290-2295
+ if (BuildOpts.AddTemporaryDtors) {
+ TempDtorContext Context;
+ VisitForTemporaryDtors(EC->getSubExpr(),
+ /*ExternallyDestructed=*/true, Context);
+ }
+ return Visit(EC->getSubExpr(), asc);
----------------
NoQ wrote:
> This looks like a copy-paste from `VisitExprWithCleanups` with the
> `ExternallyDestructed` flag flipped. Could you double-check if the update to
> `asc` that's present in `VisitExprWithCleanups` is relevant here as well?
> I've actually no idea what do these do and we don't seem to have any tests
> for them, so feel free to ignore, but it might make sense to at least
> deduplicate the code.
Oops. I omitted it because the function is always called with `AlwaysAdd`, but
should have removed the second parameter to reflect that. However, on
reflection, I think it would be better to simply add an `ExternallyDestructed`
parameter to `Visit` and `VisitExprWithCleanups`. I'll do that in the updated
patch.
================
Comment at: test/Analysis/more-dtors-cfg-output.cpp:3
+// RUN: %clang_analyze_cc1 -analyzer-checker=debug.DumpCFG -std=c++14
-DCXX2A=0 -fblocks -Wall -Wno-unused -Werror %s > %t.14 2>&1
+// RUN: %clang_analyze_cc1 -analyzer-checker=debug.DumpCFG -std=c++2a
-DCXX2A=1 -fblocks -Wall -Wno-unused -Werror %s > %t.17 2>&1
+// RUN: FileCheck --input-file=%t.14 -check-prefixes=CHECK,CXX14
-implicit-check-not=destructor %s
----------------
NoQ wrote:
> Did you mean `t.20`? :)
Yep, will fix.
================
Comment at: test/Analysis/more-dtors-cfg-output.cpp:50
+// CXX14: (CXXConstructExpr{{.*}}, struct Foo)
+// CXX14: ~Foo() (Temporary object destructor)
+// CHECK: ~Foo() (Implicit destructor)
----------------
NoQ wrote:
> Maybe also add `CXX20-NOT` so that to make sure that the destructor *isn't*
> there? (i think this applies to a lot of other tests)
The `-implicit-check-not=destructor` handles that: it complains if any line
containing "destructor" isn't covered by a CHECK.
================
Comment at: test/Analysis/more-dtors-cfg-output.cpp:93
+
+void elided_lambda_capture_init() {
+ // The copy from get_foo() into the lambda should be elided. Should call
----------------
NoQ wrote:
> Pls mention somehow that this language feature is called "generalized lambda
> captures" because it's fairly hard to google for :)
Okay :)
================
Comment at: test/Analysis/more-dtors-cfg-output.cpp:233
+
+// FIXME: Here are some cases that are currently handled wrongly:
+
----------------
NoQ wrote:
> I'm afraid that nobody will notice this comment when more items will be added
> to this test file. Having a `FIXME` in tests themselves is sufficient.
Okay, I'll remove it.
================
Comment at: test/Analysis/more-dtors-cfg-output.cpp:251-252
+void default_ctor_with_default_arg() {
+ // FIXME: The CFG records a construction of the array type but a destruction
+ // of the base type, which is inconsistent. It really should should be
+ // emitting a loop, which should also contain the construction/destruction of
----------------
NoQ wrote:
> Is it just mismatching dump styles or is it an actual error?
Oops – it's actually just the printing code for implicit destructors that does:
```
if (const ArrayType *AT = ACtx.getAsArrayType(T))
T = ACtx.getBaseElementType(AT);
```
I'm not sure why that's there: perhaps because `~Foo[]()` is not valid C++
syntax, but there's plenty of other places in the printing code that can print
invalid syntax, and printing just `~Foo()` is misleading.
For now, until this code is overhauled to emit loops for constructors and
destructors, I think it makes the most sense to remove those two lines. I'll
do that in the updated patch.
================
Comment at: test/Analysis/more-dtors-cfg-output.cpp:252-254
+ // of the base type, which is inconsistent. It really should should be
+ // emitting a loop, which should also contain the construction/destruction of
+ // default arguments.
----------------
NoQ wrote:
> These should most likely be two separate loops: default arguments are
> destroyed immediately after the constructor of `qux[2]`, but elements of
> `qux` should not be destroyed before the end of the scope.
Yeah, that's what I meant. I'll clarify the text.
================
Comment at: test/Analysis/more-dtors-cfg-output.cpp:273
+#if CXX2A
+// Boilerplate needed to test co_return:
+
----------------
NoQ wrote:
> Feel free to move this into
> `test/Analysis/inputs/system-header-simulator-cxx.h`, we could always use
> more mocks in there.
Well, `TestPromise` is a bit specific to this test. In particular, the
`return_value` method would normally return `void`, but I have it return a
`Foo`, which just gets discarded, in order to check that the corresponding
destructor call is recorded properly.
The non-test-specific boilerplate is <10 lines of code. I could add it to
`system-header-simulator-cxx.h`, but then I'd have to figure out how to make
sure that this test file doesn't get confused by CFG output from any inline
functions defined there, especially given the `-implicit-check-not` flag.
Right now almost all function implementations in the header are within a
template and thus won't produce CFG (or IR or other analysis) output by
themselves, so the solution would probably consist of merely adding a comment
to `system-header-simulator-cxx.h`, requesting that any future additions be
either similarly templated or hidden behind an #ifdef. That's definitely
feasible, but it doesn't seem worth dealing with for such a small amount of
code. :)
However, I need to fix the indentation and improve the wording of the comment
in `coreturn`.
Repository:
rC Clang
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D66404/new/
https://reviews.llvm.org/D66404
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
