This revision was automatically updated to reflect the committed changes.
Closed by commit rL339291: Use rich mangling information in
Symtab::InitNameIndexes() (authored by stefan.graenitz, committed by ).
Herald added a subscriber: llvm-commits.
Changed prior to commit:
jingham accepted this revision.
jingham added a comment.
Looks good.
https://reviews.llvm.org/D50071
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
sgraenitz marked an inline comment as done.
sgraenitz added inline comments.
Comment at: source/Core/Mangled.cpp:239-240
+//--
+namespace {
+
+char *GetMSVCDemangledStr(const char *M) {
jingham
sgraenitz updated this revision to Diff 159767.
sgraenitz marked 5 inline comments as done.
sgraenitz added a comment.
Addressed Jim's feedback
https://reviews.llvm.org/D50071
Files:
include/lldb/Core/Mangled.h
include/lldb/Core/RichManglingContext.h
include/lldb/Symbol/Symtab.h
jingham added a comment.
You and Pavel have done great work getting this all straight. I don't have
much to add except some trivial quibbles about variable naming, a grammar error
and some other inessentials.
Comment at: include/lldb/Core/Mangled.h:323-325
+ /// @return
+
labath accepted this revision.
labath added a comment.
This revision is now accepted and ready to land.
I think this looks great. I am very happy with the final result. Thank you for
your patience, and thanks again for taking on this task.
https://reviews.llvm.org/D50071
sgraenitz marked an inline comment as done.
sgraenitz added a comment.
Tests, polishing and renaming (as discussed with @jingham).
If you have more comments on this, please let me know.
Otherwise I'd be fine with keeping it like this.
https://reviews.llvm.org/D50071
sgraenitz updated this revision to Diff 159678.
sgraenitz added a comment.
Polishing
https://reviews.llvm.org/D50071
Files:
include/lldb/Core/Mangled.h
include/lldb/Core/RichManglingContext.h
include/lldb/Symbol/Symtab.h
include/lldb/lldb-forward.h
lldb.xcodeproj/project.pbxproj
labath added inline comments.
Comment at: source/Core/RichManglingContext.cpp:133
+ case PluginCxxLanguage:
+m_cxx_method_str = ConstString(
+
get(m_cxx_method_parser)->GetBasename());
sgraenitz wrote:
> labath wrote:
> > I thought we were going to
sgraenitz added inline comments.
Comment at: source/Core/RichManglingContext.cpp:133
+ case PluginCxxLanguage:
+m_cxx_method_str = ConstString(
+
get(m_cxx_method_parser)->GetBasename());
labath wrote:
> I thought we were going to get rid of this
sgraenitz updated this revision to Diff 159654.
sgraenitz added a comment.
Add unit tests for RichManglingContext
https://reviews.llvm.org/D50071
Files:
include/lldb/Core/Mangled.h
include/lldb/Core/RichManglingContext.h
include/lldb/Symbol/Symtab.h
include/lldb/lldb-forward.h
labath added inline comments.
Comment at: source/Core/RichManglingContext.cpp:133
+ case PluginCxxLanguage:
+m_cxx_method_str = ConstString(
+
get(m_cxx_method_parser)->GetBasename());
I thought we were going to get rid of this ConstString? The C++
sgraenitz added a comment.
> I'm also wondering whether it wouldn't be good to add a couple unit tests for
> the new class, as a way to show the way the API is supposed to be used
This is still missing, but working on it.
https://reviews.llvm.org/D50071
sgraenitz updated this revision to Diff 159556.
sgraenitz marked 5 inline comments as done.
sgraenitz added a comment.
Many small things
https://reviews.llvm.org/D50071
Files:
include/lldb/Core/Mangled.h
include/lldb/Core/RichManglingContext.h
include/lldb/Symbol/Symtab.h
sgraenitz added a comment.
Ok I will fix the details, rebase and update the review.
Btw: I also spilt off the `ConstString::IsNull()` addition with unit test and
fix in `Mangled` to https://reviews.llvm.org/D50327
Comment at: source/Core/RichManglingInfo.cpp:126
+ case
labath added a comment.
I think we're getting there, but all these changes mean I have another round of
comments. None of them should be major though.
I'm also wondering whether it wouldn't be good to add a couple (one for each
mangling scheme?) unit tests for the new class, as a way to show
sgraenitz added a comment.
I quickly realized, that it needs a bigger change to properly address the
aforementioned issues. Here's the major changes. Some documentation is todo.
**StringRef to reused buffer** addressed with a split into first parse then get
buffer. It seems more natural that
sgraenitz updated this revision to Diff 159066.
sgraenitz added a comment.
Polishing
https://reviews.llvm.org/D50071
Files:
include/lldb/Core/Mangled.h
include/lldb/Core/RichManglingInfo.h
include/lldb/Symbol/Symtab.h
include/lldb/Utility/ConstString.h
include/lldb/lldb-forward.h
sgraenitz updated this revision to Diff 159065.
sgraenitz added a comment.
Polishing
https://reviews.llvm.org/D50071
Files:
include/lldb/Core/Mangled.h
include/lldb/Core/RichManglingInfo.h
include/lldb/Symbol/Symtab.h
include/lldb/Utility/ConstString.h
include/lldb/lldb-forward.h
sgraenitz added inline comments.
Comment at: source/Core/RichManglingInfo.cpp:69-80
+const char *RichManglingInfo::GetFunctionBaseName() const {
+ switch (m_provider) {
+ case ItaniumPartialDemangler:
+if (auto buf = m_IPD->getFunctionBaseName(m_IPD_buf, _IPD_size)) {
+
labath added inline comments.
Comment at: include/lldb/Core/RichManglingInfo.h:36
+ /// "a::b".
+ llvm::StringRef GetFunctionDeclContextName() const;
+
sgraenitz wrote:
> I realized that returning `llvm::StringRef` from here may not be the best
> idea. To the
sgraenitz added inline comments.
Comment at: include/lldb/Core/RichManglingInfo.h:36
+ /// "a::b".
+ llvm::StringRef GetFunctionDeclContextName() const;
+
I realized that returning `llvm::StringRef` from here may not be the best idea.
To the outside it
sgraenitz added a comment.
> (However if you are interested in something like this, then it might be
> interesting to look at whether this MethodName stuff couldn't be properly
> pluginified. Something like where a Language class registers a callback for a
> specific mangling type, and then
labath added a comment.
In https://reviews.llvm.org/D50071#1184960, @sgraenitz wrote:
> > If I understand things correctly, we could avoid circular deps and untyped
> > pointers (or llvm::Any, which is essentially the same thing), by moving
> > CPlusPlusLanguage::MethodName to a separate file.
sgraenitz added a comment.
> If I understand things correctly, we could avoid circular deps and untyped
> pointers (or llvm::Any, which is essentially the same thing), by moving
> CPlusPlusLanguage::MethodName to a separate file.
Banning public nested classes in general is a good practice as
sgraenitz marked 2 inline comments as done.
sgraenitz added inline comments.
Comment at: include/lldb/Core/RichManglingInfo.h:83-84
+public:
+ RichManglingInfo *SetItaniumInfo();
+ RichManglingInfo *SetLegacyCxxParserInfo(const ConstString );
+
labath wrote:
>
sgraenitz updated this revision to Diff 158638.
sgraenitz added a comment.
Minor improvements.
https://reviews.llvm.org/D50071
Files:
include/lldb/Core/Mangled.h
include/lldb/Core/RichManglingInfo.h
include/lldb/Symbol/Symtab.h
include/lldb/Utility/ConstString.h
labath added inline comments.
Comment at: include/lldb/Core/RichManglingInfo.h:83-84
+public:
+ RichManglingInfo *SetItaniumInfo();
+ RichManglingInfo *SetLegacyCxxParserInfo(const ConstString );
+
I find it odd that one of these functions is stateless
labath added a comment.
Thank you for updating the patch.
If I understand things correctly, we could avoid circular deps and untyped
pointers (or `llvm::Any`, which is essentially the same thing), by moving
`CPlusPlusLanguage::MethodName` to a separate file. Could we do that as a
preparatory
sgraenitz created this revision.
sgraenitz added reviewers: labath, jingham, JDevlieghere, erik.pilkington.
I set up a new review, because not all the code I touched was marked as a
change in old one anymore.
In preparation for this review, there were two earlier ones:
-
30 matches
Mail list logo
