NoQ accepted this revision. NoQ added a comment. This revision is now accepted and ready to land.
This looks fantastic, thanks! > Note: I don't have commit access. I can commit this for you but i believe you should totally ask for it <https://llvm.org/docs/DeveloperPolicy.html#obtaining-commit-access>. ================ Comment at: test/Analysis/more-dtors-cfg-output.cpp:50 +// CXX14: (CXXConstructExpr{{.*}}, struct Foo) +// CXX14: ~Foo() (Temporary object destructor) +// CHECK: ~Foo() (Implicit destructor) ---------------- comex wrote: > 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. Ahaa, i see what you did here! ================ 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 ---------------- comex wrote: > 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. I don't care if it's valid C++ syntax as long as it's readable/testable. Another possible variant is `~Foo() (Array destructor)`, we seem to do this a lot. ================ Comment at: test/Analysis/more-dtors-cfg-output.cpp:273 +#if CXX2A +// Boilerplate needed to test co_return: + ---------------- comex wrote: > 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`. Ok np then! 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
