[Lldb-commits] [PATCH] D35556: Add data formatter for libc++'s forward_list

[Jim Ingham via Phabricator via lldb-commits]

jingham accepted this revision.
jingham added a comment.
This revision is now accepted and ready to land.

Great, this is much nicer.



================
Comment at: source/Plugins/Language/CPlusPlus/LibCxxList.cpp:141
 
-namespace lldb_private {
-namespace formatters {
-class LibcxxStdListSyntheticFrontEnd : public SyntheticChildrenFrontEnd {
+class ForwardListFrontEnd: public AbstractListFrontEnd {
+public:
----------------
labath wrote:
> jingham wrote:
> > I wonder about the decision to move these classes out of lldb_private::formatters.  They don't need to be in a globally visible namespace, but all the others are.  They also all have a consistent naming convention, which this one doesn't have.  This doesn't seem like the sort of thing we should change piecemeal, that will just lead to confusion.
> > 
> > Was there some other reason for doing this that I'm missing?
> There's no hard reason, but I was trying to make these names more concise
> 
> I've put them in the anonymous namespace, as llvm coding style encourages that for file-local entities <https://llvm.org/docs/CodingStandards.html#anonymous-namespaces>. I'd be happy to move the other formatters into the anonymous namespace as well, as I have a couple of other formatters in the pipeline, which already are in the anon namespace.
> 
> As for the names, if I followed the existing style, this would read something like
> ```
> class LibCxxStdForwardListSyntheticFrontEnd: public LibCxxAbstractListSyntheticFrontEnd
> ```
> which would be formatted horribly, as it does not fit a single line. As these are file-local entities, I think we can be a bit less verbose.
> 
> I had not renamed the std::list class because of the way I wrote this change (make a copy of the std::list formatter, and then factor out the common code), but you are right that they should follow the same convention. I propose to shorten the name of the other class as well.
The effect on choosing names is one of the many reasons why I don't favor the llvm 80 character restriction.  But we're stuck with it so...


https://reviews.llvm.org/D35556