Page MenuHomePhabricator

[NativePDB] Make GetOrCreateDeclForUid return an lldb CompilerDecl
ClosedPublic

Authored by lanza on Jul 12 2019, 8:23 PM.

Details

Summary

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.

Event Timeline

lanza created this revision.Jul 12 2019, 8:23 PM

LGTM! So the other functions (e.g. GetOrCreateDeclContextForUid, GetParentDeclContext etc.) will be also replaced in the future in a corresponding manner?

This revision is now accepted and ready to land.Jul 15 2019, 11:49 PM

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?

lanza added a comment.Jul 16 2019, 9:45 PM

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.

lanza marked 4 inline comments as done.Jul 17 2019, 12:09 AM
lanza added inline comments.
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.

lanza marked an inline comment as done.Jul 17 2019, 12:10 AM
amccarth accepted this revision.Jul 17 2019, 8:11 AM
amccarth marked an inline comment as done.

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 always returning an object, and the failure cases are now default constructed objects. That has an impact on the callers, so it seems (to me) a waste to have to change the callers twice, especially since the Expected<> version would be handled very similarly to the possibly-null pointer version.

Your call. If you're planning to do this in a future patch, I'm satisfied.

Oh, and thanks for the background and context on the motivation for this change!

lanza marked an inline comment as done.Jul 17 2019, 11:57 PM
lanza added inline comments.
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.

lanza updated this revision to Diff 210493.Jul 18 2019, 12:25 AM

optional

amccarth accepted this revision.Jul 18 2019, 8:32 AM

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.

lanza closed this revision.Sun, Jul 21, 1:45 AM