Re: [Lldb-commits] [PATCH] D51162: [PDB] Restore AST from PDB symbols

Agreed. I'll implement it tomorrow, thanks! On Tue, Sep 11, 2018 at 7:24 PM Zachary Turner wrote: > A visitor would work, but unless we need this frequently it might be > overkill. If we do need it frequently then it would be very helpful though. > > For now since we just have this one use case

Re: [Lldb-commits] [PATCH] D51162: [PDB] Restore AST from PDB symbols

A visitor would work, but unless we need this frequently it might be overkill. If we do need it frequently then it would be very helpful though. For now since we just have this one use case I think a switch statement on the tag is the simplest. You can group all same cases together and use the raw

Re: [Lldb-commits] [PATCH] D51162: [PDB] Restore AST from PDB symbols

Yes, you are right, we can just to consider some cases important for us in the function, but I thought to solve the problem in a more general way. I think that the key problem is that we lose some type info when we get a `PDBSymbol`. So we need to dyn_cast the symbol multiple times or to analyse t

[Lldb-commits] [PATCH] D51162: [PDB] Restore AST from PDB symbols

llvm-commits added a subscriber: labath. llvm-commits added a comment. That’s fine - F7177739: msg-17534-333.txt Repository: rL LLVM https://reviews.llvm.org/D51162 ___ lldb-commits mailing list lldb-commits@l

Re: [Lldb-commits] [PATCH] D51162: [PDB] Restore AST from PDB symbols

On Tue, Sep 11, 2018 at 4:28 AM Aleksandr Urakov via Phabricator < revi...@reviews.llvm.org> wrote: > aleksandr.urakov added inline comments. > > > > Comment at: lldb/trunk/source/Plugins/SymbolFile/PDB/PDBASTParser.cpp:289 > + return GetClassOrFunctionParent(*lexical_parent); >

Re: [Lldb-commits] [PATCH] D51162: [PDB] Restore AST from PDB symbols

That’s fine On Tue, Sep 11, 2018 at 7:11 AM Aleksandr Urakov via Phabricator < revi...@reviews.llvm.org> wrote: > aleksandr.urakov added a comment. > > @stella.stamenova @ted Fixed with https://reviews.llvm.org/rL341942, > thanks again! > @zturner I'll rewrite `GetClassOrFunctionParent` and will c

[Lldb-commits] [PATCH] D51162: [PDB] Restore AST from PDB symbols

aleksandr.urakov added a comment. @stella.stamenova @ted Fixed with https://reviews.llvm.org/rL341942, thanks again! @zturner I'll rewrite `GetClassOrFunctionParent` and will create a different review for that, ok? Repository: rL LLVM https://reviews.llvm.org/D51162 _

[Lldb-commits] [PATCH] D51162: [PDB] Restore AST from PDB symbols

aleksandr.urakov added a comment. In https://reviews.llvm.org/D51162#1229112, @stella.stamenova wrote: > This change is causing three of the symbol file PDB tests to fail: > > lldb-Unit :: > SymbolFile/PDB/release/SymbolFilePDBTests.exe/SymbolFilePDBTests.TestClassInNamespace > lldb-Unit ::

[Lldb-commits] [PATCH] D51162: [PDB] Restore AST from PDB symbols

aleksandr.urakov added inline comments. Comment at: lldb/trunk/source/Plugins/SymbolFile/PDB/PDBASTParser.cpp:289 + return GetClassOrFunctionParent(*lexical_parent); +} + zturner wrote: > However, this is only for symbols (not types). So we can exclude everythi

[Lldb-commits] [PATCH] D51162: [PDB] Restore AST from PDB symbols

aleksandr.urakov added inline comments. Comment at: lldb/trunk/source/Plugins/SymbolFile/PDB/PDBASTParser.cpp:268-269 + +std::unique_ptr +GetClassOrFunctionParent(const llvm::pdb::PDBSymbol &symbol) { + const IPDBSession &session = symbol.getSession(); zturner w

[Lldb-commits] [PATCH] D51162: [PDB] Restore AST from PDB symbols

aleksandr.urakov added a comment. In https://reviews.llvm.org/D51162#1229616, @ted wrote: > The "auto from = 0" should be "size_t from = 0", since auto can't determine > the correct type. Yes, I've missed that because MSVC doesn't emit such a warning. Thanks for catching that! Repository:

[Lldb-commits] [PATCH] D51162: [PDB] Restore AST from PDB symbols

zturner added inline comments. Comment at: lldb/trunk/source/Plugins/SymbolFile/PDB/PDBASTParser.cpp:268-269 + +std::unique_ptr +GetClassOrFunctionParent(const llvm::pdb::PDBSymbol &symbol) { + const IPDBSession &session = symbol.getSession(); All file local fun

[Lldb-commits] [PATCH] D51162: [PDB] Restore AST from PDB symbols

ted added a comment. Another issue: auto context = symbol.getRawSymbol().getName(); auto context_size = context.rfind("::"); ... auto from = 0; while (from < context_size) { context_size is size_t (from std::string::rfind), but on clang 5.01, "auto from = 0" makes from an int. The

[Lldb-commits] [PATCH] D51162: [PDB] Restore AST from PDB symbols

aleksandr.urakov added a comment. Sorry, I've missed this. I'll fix it tomorrow. Thank you! Repository: rL LLVM https://reviews.llvm.org/D51162 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listin

[Lldb-commits] [PATCH] D51162: [PDB] Restore AST from PDB symbols

stella.stamenova added a comment. This change is causing three of the symbol file PDB tests to fail: lldb-Unit :: SymbolFile/PDB/release/SymbolFilePDBTests.exe/SymbolFilePDBTests.TestClassInNamespace lldb-Unit :: SymbolFile/PDB/release/SymbolFilePDBTests.exe/SymbolFilePDBTests.TestNestedCla

[Lldb-commits] [PATCH] D51162: [PDB] Restore AST from PDB symbols

This revision was automatically updated to reflect the committed changes. Closed by commit rL341782: [PDB] Restore AST from PDB symbols (authored by aleksandr.urakov, committed by ). Herald added a subscriber: llvm-commits. Changed prior to commit: https://reviews.llvm.org/D51162?vs=164027&id=1

[Lldb-commits] [PATCH] D51162: [PDB] Restore AST from PDB symbols

aleksandr.urakov added a comment. Thanks all! https://reviews.llvm.org/D51162 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D51162: [PDB] Restore AST from PDB symbols

asmith accepted this revision. asmith added a comment. This revision is now accepted and ready to land. LGTM! https://reviews.llvm.org/D51162 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/ll

[Lldb-commits] [PATCH] D51162: [PDB] Restore AST from PDB symbols

zturner added a comment. Lgtm, thanks! https://reviews.llvm.org/D51162 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Re: [Lldb-commits] [PATCH] D51162: [PDB] Restore AST from PDB symbols

Lgtm, thanks! On Wed, Sep 5, 2018 at 6:43 AM Aleksandr Urakov via Phabricator < revi...@reviews.llvm.org> wrote: > aleksandr.urakov updated this revision to Diff 164027. > aleksandr.urakov added a comment. > > Code cleanup > > > https://reviews.llvm.org/D51162 > > Files: > include/lldb/Symbol/Cl

[Lldb-commits] [PATCH] D51162: [PDB] Restore AST from PDB symbols

aleksandr.urakov updated this revision to Diff 164026. aleksandr.urakov added a comment. - Fix a typo bug in `AddRecordMethods` (use `continue` instead of `break`); - Do not search function declarations by name during getting of a declaration for a symbol, it may lead to ambiguity. https://revi

[Lldb-commits] [PATCH] D51162: [PDB] Restore AST from PDB symbols

aleksandr.urakov updated this revision to Diff 164027. aleksandr.urakov added a comment. Code cleanup https://reviews.llvm.org/D51162 Files: include/lldb/Symbol/ClangASTContext.h lit/SymbolFile/PDB/Inputs/AstRestoreTest.cpp lit/SymbolFile/PDB/ast-restore.test lit/SymbolFile/PDB/class-la

[Lldb-commits] [PATCH] D51162: [PDB] Restore AST from PDB symbols

aleksandr.urakov updated this revision to Diff 163708. aleksandr.urakov added a comment. Added the dumping AST ability to `lldb-test`; adding a corresponding test for the patch. https://reviews.llvm.org/D51162 Files: include/lldb/Symbol/ClangASTContext.h lit/SymbolFile/PDB/Inputs/AstRestor

[Lldb-commits] [PATCH] D51162: [PDB] Restore AST from PDB symbols

aleksandr.urakov added a comment. Yes, I'll try to implement that, thanks for the idea! https://reviews.llvm.org/D51162 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D51162: [PDB] Restore AST from PDB symbols

zturner added a comment. I think in clang they have a method to dump the AST. Could we add something like that to `lldb-test`? We're using the `symbols` subcommand, maybe it would be worth it to add a `--dump-ast` flag to that subcommand? That seems like a generally useful testing feature.

[Lldb-commits] [PATCH] D51162: [PDB] Restore AST from PDB symbols

aleksandr.urakov added a comment. Thanks :) Let's also wait for what Aaron will say? Because the patch is big enough. As for testing, what do you think if I'll add complex tests for the expressions evaluation, when it will be ready? I have no other idea how to test the changes in this patch...

Re: [Lldb-commits] [PATCH] D51162: [PDB] Restore AST from PDB symbols

I don’t think I’m really a good person to look at AST stuff. I can look for general style comments, obvious flaws, and test coverage. but you may be the best person regarding on the content of the patch. Does that sound ok? On Fri, Aug 31, 2018 at 7:20 AM Aleksandr Urakov via Phabricator < revi...@

[Lldb-commits] [PATCH] D51162: [PDB] Restore AST from PDB symbols

zturner added a subscriber: aleksandr.urakov. zturner added a comment. I don’t think I’m really a good person to look at AST stuff. I can look for general style comments, obvious flaws, and test coverage. but you may be the best person regarding on the content of the patch. Does that sound ok? h

[Lldb-commits] [PATCH] D51162: [PDB] Restore AST from PDB symbols

aleksandr.urakov updated this revision to Diff 163528. aleksandr.urakov added a comment. Drop a scope part of a class member function name. https://reviews.llvm.org/D51162 Files: include/lldb/Symbol/ClangASTContext.h lit/SymbolFile/PDB/class-layout.test lit/SymbolFile/PDB/func-symbols.tes

[Lldb-commits] [PATCH] D51162: [PDB] Restore AST from PDB symbols

aleksandr.urakov added a comment. Herald added a subscriber: teemperor. Ping! Can you review this, please? Repository: rLLDB LLDB https://reviews.llvm.org/D51162 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi