[PATCH] D35159: [libcxxabi][demangler] Use an AST to represent the demangled name

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith added a reviewer: mclow.lists.
dexonsmith added a subscriber: mclow.lists.
dexonsmith added a comment.

+mclow.lists



================
Comment at: src/cxa_demangle.cpp:44
 
+class string_ref
+{
----------------
mehdi_amini wrote:
> dexonsmith wrote:
> > erik.pilkington wrote:
> > > mehdi_amini wrote:
> > > > If this is supposed to be *the* ultimate LLVM demangler, can we follow LLVM coding standard?
> > > I would like if this followed LLVM conventions too, but this file is already written following this style and leaving it in some middle state would be ugly. All of libcxx[abi] follows this convention too, so this isn't a problem that is isolated to this file.
> > I agree.  I'd be fine with clang-formatting the entire project, but that seems independent from this change.
> I'm not talking about pure "clang-format" but also naming for instance.
> 
> > I would like if this followed LLVM conventions too, but this file is already written following this style and leaving it in some middle state would be ugly.
> 
> Right, but in the meantime you're adding a significant amount of "debt".
> 
> > All of libcxx[abi] follows this convention too, so this isn't a problem that is isolated to this file.
> 
> This file is "special": it is shared (duplicated...) with LLVM.
> 
This is a patch for libcxxabi.  Duplicating the file in LLVM is what created technical debt, and that file has already diverged from libcxxabi.  That's where the bug is.

AFAICT, there is no requirement for LLVM subprojects to use LLVM naming schemes, and since libcxx and libcxxabi are implementing STL facilities, it's reasonable for them to use STL naming conventions (even in private implementations that aren't exposed to users).

(It would also be reasonable to follow LLVM naming conventions in private implementations, but that's not the current practice, and it would certainly inhibit code readability here to do so just for one type.)

Perhaps a compromise would be to rename `string_ref` to `string_view`, so that it sounds more like the equivalent STL type than the equivalent LLVM type.

@mclow.lists, would you like to weigh in as code owner here?  Should the naming scheme for new types in libcxxabi private implementations follow LLVM coding conventions, or libcxxabi coding conventions?


https://reviews.llvm.org/D35159