Page MenuHomePhabricator

CPlusPlusLanguage: Use new demangler API to implement type substitution

Authored by labath on Nov 3 2018, 11:31 AM.



Now that llvm demangler supports more generic customization, we can
implement type substitution directly on top of this API. This will allow
us to remove the specialized hooks which were added to the demangler to
support this use case.

Diff Detail


Event Timeline

labath created this revision.Nov 3 2018, 11:31 AM

Hi Pavel, thanks for working on this. The code looks really great. I have no actual concerns about landing it like this.

Two minor remarks:

  • We still have no coverage for substitution failures in CPlusPlusLanguageTest.cpp test case FindAlternateFunctionManglings.
  • We create a new BumpPtrAllocator for each TypeSubstitutor::substitute(), right? Well, the impact on performance is probably on a different order of magnitude than we are concerned about here.

This will allow us to remove the specialized hooks which were added to the demangler to support this use case.

Right, it bridged the time between removing FastDemangle from LLDB and getting the new ManglingParser in LLVM. IIUC this basically means reverting

labath updated this revision to Diff 172583.Nov 5 2018, 6:54 AM

Thanks for the review.

I think that creating the new BumpPtrAllocator every call doesn't matter much here, this function
get's called only on a few symbols for each evaluated expression. However, it also wasn't too
hard to reuse it (at least at the level of FindAlternateFunctionManglings), so I just did that.

I also added a simple failure test for this function.

sgraenitz accepted this revision.Nov 5 2018, 7:38 AM

Nice. For parameter lists of reset and substitute indentation could be fixed, but otherwise this looks really good to me.

This revision is now accepted and ready to land.Nov 5 2018, 7:38 AM

Looks good to me too, thanks!

shafik added a subscriber: shafik.Nov 5 2018, 2:13 PM
shafik added inline comments.
336 ↗(On Diff #172583)

Can you just use numLeft() here? It would be less opaque.

337 ↗(On Diff #172583)

also First - Written is used twice but it is not obvious what the means, naming it via a member function might be helpful.

JDevlieghere accepted this revision.Nov 5 2018, 6:49 PM
sgraenitz added inline comments.Nov 6 2018, 3:25 AM
337 ↗(On Diff #172583)

First points to the first char of the input (Mangled).
Written points to the char of the input, up until we have constructed the respective output already.

Quite similar to the previous implementation. Difference is: it used a std::back_insert_iterator, but I wouldn't say that made it simpler. Intelligibility may benefit from comment as in the past:

// Write the unmatched part of the original. Then write the
// replacement string. Finally skip the search string in the original.

IMHO it's not a high prio.

labath updated this revision to Diff 172764.Nov 6 2018, 6:57 AM

Thanks for the review. I have added some comments, and moved appending of the unmodified portion of the name into a separate function to aid readability.

This revision was automatically updated to reflect the committed changes.

Much better, thank you!