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
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
