We intend to make PdbAstBuilder abstract and implement
PdbAstBuilderClang along with any other languages that wish to use
PDBs. Thus, change GetOrCreateDeclForUid from returning a clang decl
to a lldb_private::CompilerDecl.
Details
- Reviewers
zturner aleksandr.urakov amccarth
Diff Detail
- Build Status
Buildable 35225 Build 35224: arc lint + arc unit
Event Timeline
LGTM! So the other functions (e.g. GetOrCreateDeclContextForUid, GetParentDeclContext etc.) will be also replaced in the future in a corresponding manner?
Was there a proposal or discussion (e.g., a thread on lldb-dev) about making PdbAstBuilder abstract in order to handle other languages? It sounds like a good idea, but I'd like to catch up on any context I may have missed.
source/Plugins/SymbolFile/NativePDB/PdbAstBuilder.cpp | ||
---|---|---|
470 | Should the return value be an Expected<lldb_private::CompilerDecl>? | |
473 | auto decl = ...; return decl; seems unnecessarily verbose. The name decl doesn't add any value beyond CompilerDecl, so consider just returning the expression. | |
490 | Again, I would just return the expression directly. | |
source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp | ||
1509–1512 | I'm a little confused here. It looks like we took a CompilerDecl apart, made an assumption about the language, and put it back together. That has no net effect, right? Is this in anticipation of future changes or should this just return decl? |
Was there a proposal or discussion (e.g., a thread on lldb-dev) about making PdbAstBuilder abstract in order to handle other languages? It sounds like a good idea, but I'd like to catch up on any context I may have missed.
Nope. We're considering using PDBs for Swift on Windows and this is the first of a long list of steps that would need to be done for it. I submitted similar things in the past to SymbolFilePDB and Zach redirected me to the native implementation, but I got pulled away from this work for some time after that. That was the limit of the discussion outside of my team.
To summarize -- PdbAstBuilder, SymbolFileNativePDB and maybe one other file need the word clang ripped out of them and replaced with the corresponding types. At that point PdbAstBuilder becomes abstract and PdbAstBuilderClang is created followed by PdbAstBuilderSwift.
LGTM! So the other functions (e.g. GetOrCreateDeclContextForUid, GetParentDeclContext etc.) will be also replaced in the future in a corresponding manner?
Yup. I was asked to split it up last time, so just sticking with that request.
source/Plugins/SymbolFile/NativePDB/PdbAstBuilder.cpp | ||
---|---|---|
470 | Similar to before, I plan on doing a pass like this once all the types are done changing. |
LGTM, but please reconsider whether the return type of GetOrCreateDeclForUid should be changed in this patch rather than in a future one.
source/Plugins/SymbolFile/NativePDB/PdbAstBuilder.cpp | ||
---|---|---|
470 | This case is a bit different, though. The original code returns pointers (and a nullptr for the failure case). Your call. If you're planning to do this in a future patch, I'm satisfied. |
source/Plugins/SymbolFile/NativePDB/PdbAstBuilder.cpp | ||
---|---|---|
470 | Sure, I'll go ahead and do them as I change the function signatures.. Though this is more fitting for Optional<> given that not having debug info is not an error. |
Thanks for the change. I was thinking Expected based on the name of the function: GetOrCreateDeclForUid. I was thinking that it would be odd to have a UID if you don't have debug info. Anyway, Optional is fine. Thanks again.
Should the return value be an Expected<lldb_private::CompilerDecl>?