[PATCH] D81389: dwarf::isCPlusPlus fails with user language

Mon Jun 8 12:08:18 PDT 2020

dblaikie added a comment.

In D81389#2079773 <https://reviews.llvm.org/D81389#2079773>, @probinson wrote:

> FTR, we like to have full context diffs as described here: https://llvm.org/docs/Phabricator.html#phabricator-request-review-web
>
> The code patch looks fine.  However, we want tests for all new features, but the patch introducing `isCPlusPlus` doesn't seem to have included an explicit test, and making you write one seems excessive.  + @dblaikie as he committed that faux pas.

Not sure what an 'explicit' test for this would look like - the function is used to determine a few things about how to handle composite names (namespaces, etc) - whether to use '::' as a path separator (llvm::DwarfUnit::getParentContextString). I'm generally happy for tests to cover unions of different behavior sometimes - if it doesn't convolute the test too much.

In any case, yeah, this change does need test coverage - if folks are using LLVM debug info for non-C++ languages and using pubnames or debug_names (which is what you'd have to be using to trip over this unreachable) you're probably going to want to look at llvm::DwarfUnit::getParentContextString that doesn't add any context for non-C++ scopes. So if your language has scoping this might be a problem for debug info consumers.

If all of that is fine/correct - then, yeah, a small test case using pubnames or debug_names that shows the namespace scopes are not included in the names of nested elements in the index would be good.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D81389/new/

https://reviews.llvm.org/D81389