- User Since
- Oct 10 2017, 8:01 AM (170 w, 4 d)
Dec 15 2020
Thanks for your work, I am glad that finally the Window bug is fixed with a simple solution.
Looks quite straight-forward other than that new prototype.
Nice and precise work! And special thanks for the unit tests. I've found some minor nits.
Dec 14 2020
Shafik, thanks for the review!
Thanks for the update. I checked it, still looks good to me.
Dec 10 2020
- Check the attribute in the test as well
I wouldn't worry much about the ASTMerge tests, those are mostly legacy and not actively used nowadays. (At one point we were thinking even to remove them because we could got rid of the ASTMergeAction which is exclusively used for testing.)
The main test infrastructure for the ASTImporter is in the unittests. Those tests are exercised even with -fdelayed-template-parsing and with -fms-compatibility. So I am fine with marking the ASTMerge tests with XFAIL on windows.
Dec 9 2020
- Remove not relevant param from test
However, every single TU in LLDB starts being pretty much empty (we do have some dummy declarations in them, but none of them have any name conflicts with declarations in any other AST we import from). So technically the 'empty' LookupTable is accurate.
This is the case if you create the SharedState without a lookup table. If you pass a TUDecl then a lookup table will be created and in the constructor of that we traverse the AST to initialize the lookup. And from that point we will find every existing Decl even if they were not imported. As it is now we'd still be using the old noload_lookup.
Thanks for the review guys!
- Remove if (!DC || !LexicalDC)
Dec 8 2020
Thanks for the review!
Dec 7 2020
@teemperor please take a look if you have some time. This is a really important patch which may influence some future directions regarding the ASTImporter.
- Use -1 for mmap too
Dec 4 2020
It might be worth to extend StmtComparer as well with this Expr. I mean, probably two selections exprs should be considered equal if their assoc exprs are equal too...
Dec 3 2020
I was thinking about to create a separate ASTImporter implementation specifically for CTU and LLDB could have it's own implementation. For that we just need to create an interface class with virtual functions.
Dec 2 2020
- Remove comments
- Hoist Range(-1,0) to ReturnsZeroOrMinusOne
Looks good, thanks!
Looks good, thanks!
Hey Raphael, thanks for looking into the CTU crash!
@rsmith I have moved the check into TransformTypedefType. We do not add the typedef to the MaterializedTypedefs list if it's context is dependent because later we set the deduction guide as the DC for all typedefs in the list:
for (auto *TD : MaterializedTypedefs) TD->setDeclContext(Guide);
- Add to MaterializedTypedefs only when copying the Decl
- Move dependent context check into TransformTypedefType
- Add new test case for param with pointer type
Dec 1 2020
Nov 30 2020
- Check for dependent DeclContext
- Remove "dump()" calls from tests
Looks good, thanks!
Nov 27 2020
- Update test to match the copy deduction guide
Nov 26 2020
Thanks for the review! Your comment made me think about whether we handle the deduced template class properly (getDeducedTemplate). Then I realized we do import that when we import the DeclarationName. Anyway I added a check for that in the test.
- Add test for user-defined ctad
- Handle IsCopyDeductionCandidate
- Fix typo
Looks good, thanks!
Nov 25 2020
This looks good as well, thanks!
Nov 24 2020
LGTM! And thanks for the pro unittest!
Nov 20 2020
Thanks, looks good to me!
Nov 19 2020
Nov 17 2020
Nov 5 2020
We are trying to write checkers to catch possibly zero state.
IMO, that should be handled with taint analysis, i.e. when the value's provenance is untrusted we should warn. I don't see any other cases when we'd like to warn about a possible 0 denominator because that would cause false positives.
Nov 3 2020
Looks good to me now! Thanks for addressing my concerns.
Oct 30 2020
Oct 28 2020
FYI, I am working on repeating and reevaluating Peter's measurements with the current master. And if possible then I'd like to extend the measures for new C and C++ projects. I am planning to share the statistics and run-time results.
Fair enough. But I don't think that Clang developers just copied the implementation from GCC or from MSVC.
I accept to keep the interface of the feature (i.e. the YAML format) as it is in the fork, okay. But, I'd like to have maximum flexibility from the submitter by willing to change the details of the implementation. I don't think that the implementation in the fork is necessarily the best realization, we can do better, after all that's the purpose of this review, isn't it?
Oct 27 2020
Now I'm going to make unittests for SValBuilder::evalCast. Let me ask you to offer me some examples of casts to check. As many cases we can collect as robust the test would be. E.g.
Oct 20 2020
I was thinking about an alternative approach (5th approach (?), yeah, I think we need an RFC).
Ok, thanks for the context. LG!
Unfortunately, I could not reproduce the mentioned crash on our macOS machine. The mentioned test just passed with the output below. I gave up.
myuser@msmarple ~/llvm3/build/release_assert $ /usr/local/Frameworks/Python.framework/Versions/3.8/bin/python3.8 /Users/myuser/llvm3/git/llvm-project/lldb/test/API/dotest.py -S nm -u CXXFLAGS -u CFLAGS --codesign-identity - --env LLVM_LIBS_DIR=/Users/myuser/llvm3/build/release_assert/./lib --arch x86_64 --build-dir /Users/myuser/llvm3/build/release_assert/lldb-test-build.noindex -s /Users/myuser/llvm3/build/release_assert/lldb-test-traces --lldb-module-cache-dir /Users/myuser/llvm3/build/release_assert/lldb-test-build.noindex/module-cache-lldb/lldb-api --clang-module-cache-dir /Users/myuser/llvm3/build/release_assert/lldb-test-build.noindex/module-cache-clang/lldb-api --executable /Users/myuser/llvm3/build/release_assert/./bin/lldb --compiler /Users/myuser/llvm3/build/release_assert/./bin/clang --dsymutil /Users/myuser/llvm3/build/release_assert/./bin/dsymutil --filecheck /Users/myuser/llvm3/build/release_assert/./bin/FileCheck --yaml2obj /Users/myuser/llvm3/build/release_assert/./bin/yaml2obj --server /Users/myuser/llvm3/build/release_assert/./bin/debugserver --lldb-libs-dir /Users/myuser/llvm3/build/release_assert/./lib /Users/myuser/llvm3/git/llvm-project/lldb/test/API/commands/expression/import_builtin_fileid/ -p TestImportBuiltinFileID.py -t lldb version 12.0.99 (https://github.com/llvm/llvm-project.git revision d454328ea88562a6ec6260529a040035ab9c4a06) clang revision d454328ea88562a6ec6260529a040035ab9c4a06 llvm revision d454328ea88562a6ec6260529a040035ab9c4a06 libstdcxx tests will not be run because: Don't know how to build with libstdcxx on macosx Skipping following debug info categories: ['dwo']
Oct 19 2020
Adding @NoQ as a reviewer, he might have some valuable comments, as he is very keen on this feature.
as it is something which is actually in production already
At the very least we need it for compatibility - this is already a shipping feature.
Oct 16 2020
What is the context here? Did it cause any crash/bug or were you just reading through the code for a good night sleep? :D
Seems like this patch handles already existing attributes in Clang, so this might not be affected by the concerns brought up here by James: