[Lldb-commits] [lldb] r255364 - Fix Clang-tidy modernize-use-nullptr and readability-simplify-boolean-expr warnings in source/Target/Target.cpp.

Zachary Turner via lldb-commits lldb-commits at lists.llvm.org
Fri Dec 11 17:02:50 PST 2015


Agree, no change in coding standard should or would ever be accompanied by
a wholesale change.  It's more just agreeing on what clang-format can
automatically do for us, and then everyone using clang-format.  Note that
if you use the git clang-format extension (or an equivalent svn one if one
exists) it only formats the lines in your diff, not the full content of
each file you touch.  So this is o nly ever happening to lines that were
dirty anyway as a result of your CL

On Fri, Dec 11, 2015 at 4:50 PM Jim Ingham <jingham at apple.com> wrote:

> Note, it is pure speculation that this is a bug in clang-format based on
> where the : is placed.
>
> I prefer the way we write it, because it places all the initializers on
> the same level, though I don’t feel so strongly about this one.
>
> I would really rather not go through making wholesale changes like this
> unless there’s some compelling reason as it makes the history harder to
> trace by inserting random inessential checkins that you have to peer
> through.  And this seems like make-work to me.
>
> Jim
>
>
>
> On Dec 11, 2015, at 4:40 PM, Zachary Turner <zturner at google.com> wrote:
>
> Hrmm, that's interesting.  I guess I was mistaken in LLVM's rules then.
> Are we willing to accept this format in LLDB?
>
> LabelDecl(DeclContext *DC, SourceLocation IdentL, IdentifierInfo *II,
>             LabelStmt *S, SourceLocation StartL)
>     : NamedDecl(Label, DC, IdentL, II),
>       TheStmt(S),
>       MSAsmNameResolved(false),
>       LocStart(StartL) {}
>
> If so that's less work I have to do on clang-format to get it up to spec.
>
> On Fri, Dec 11, 2015 at 4:34 PM Jim Ingham <jingham at apple.com> wrote:
>
>> On Dec 11, 2015, at 2:42 PM, Zachary Turner <zturner at google.com> wrote:
>>
>> Yes, but as I mentioned, two things are still unsupported due to
>> limitations in clang-format.  They are return-type-on-new-line (only in
>> declarations.  clang-format supports it for definitions) and the
>> constructor initializer list comma at the end (clang-format puts it at the
>> beginning).
>>
>> That said, the comma at the end of initializer list isn't documented on
>> that page, and where we don't have a clearly documented rule, prefer the
>> LLVM guidelines, so….
>>
>>
>> That clang-format behavior seems weird to me, a quick scan through clang
>> sources have the commas always at the end:
>>
>>   LabelDecl(DeclContext *DC, SourceLocation IdentL, IdentifierInfo *II,
>>             LabelStmt *S, SourceLocation StartL)
>>     : NamedDecl(Label, DC, IdentL, II),
>>       TheStmt(S),
>>       MSAsmNameResolved(false),
>>       LocStart(StartL) {}
>>
>> etc.  I can’t remember seeing code in clang that does:
>>
>>   LabelDecl(DeclContext *DC, SourceLocation IdentL, IdentifierInfo *II,
>>             LabelStmt *S, SourceLocation StartL)
>>     : NamedDecl(Label, DC, IdentL, II)
>>     , TheStmt(S)
>>     , MSAsmNameResolved(false)
>>     , LocStart(StartL) {}
>>
>> That looks really weird, since you have to look past the noise of those
>> commas to see what you really care about.
>>
>> We do differ in that we tend to write this:
>>
>>   LabelDecl(DeclContext *DC, SourceLocation IdentL, IdentifierInfo *II,
>>             LabelStmt *S, SourceLocation StartL) :
>>       NamedDecl(Label, DC, IdentL, II),
>>       TheStmt(S),
>>       MSAsmNameResolved(false),
>>       LocStart(StartL) {}
>>
>> with the colon at the end of the argument list.  I don’t think any of
>> this behavior is prescribed one way or the other in the actual coding
>> conventions…  Maybe there’s a bug in clang-format that if you put the : in
>> the unexpected place the commas get moved to the wrong place as well?  But
>> the llvm coding conventions make no prescription about this at all. I don’t
>> think our code layout should be based on clang format bugs and all. We
>> certainly shouldn’t do wholesale reformats that just make history harder to
>> read for that reason.
>>
>> Jim
>>
>>
>>
>> On Fri, Dec 11, 2015 at 2:37 PM Todd Fiala <todd.fiala at gmail.com> wrote:
>>
>>> Okay, but does the format match the LLDB-modified format with some kind
>>> of configuration file?  We still need to match our guidelines here:
>>>
>>> http://lldb.llvm.org/lldb-coding-conventions.html
>>>
>>> We can achieve that with a config file for it, right?  (Maybe already
>>> existing, maybe in the lldb source tree already?)
>>>
>>> On Fri, Dec 11, 2015 at 2:35 PM, Zachary Turner <zturner at google.com>
>>> wrote:
>>>
>>>> With git you can already run "git clang-format".  You just need
>>>> `git-clang-format` to be in your PATH (it's under llvm/tools/clang).  Not
>>>> sure how to hook it into SVN
>>>>
>>>> On Fri, Dec 11, 2015 at 2:32 PM Eugene Zelenko <
>>>> eugene.zelenko at gmail.com> wrote:
>>>>
>>>>> At least clang-format should be applied to all newly added files
>>>>> before commit.
>>>>>
>>>>> Eugene.
>>>>>
>>>>> On Fri, Dec 11, 2015 at 2:30 PM, Zachary Turner <zturner at google.com>
>>>>> wrote:
>>>>> > Back on the topic of clang-format, what would it take to make
>>>>> clang-format a
>>>>> > regular part of peoples' workflows?
>>>>> >
>>>>> > On Fri, Dec 11, 2015 at 2:27 PM Todd Fiala <todd.fiala at gmail.com>
>>>>> wrote:
>>>>> >>
>>>>> >> Yep - sorry.  I had been talking to Greg about this and
>>>>> misunderstood his
>>>>> >> comment on it. My mistake entirely.  Kate and I just talked and she
>>>>> pointed
>>>>> >> me to your document, Jim.
>>>>> >>
>>>>> >> The description was:
>>>>> >> where we had a clearly adhered to standard, keep it.
>>>>> >> whee we didn't, we adopted LLVM.
>>>>> >>
>>>>> >> Sorry for rehashing!
>>>>> >>
>>>>> >> -Todd
>>>>> >>
>>>>> >> On Fri, Dec 11, 2015 at 2:12 PM, Jim Ingham <jingham at apple.com>
>>>>> wrote:
>>>>> >>>
>>>>> >>>
>>>>> >>> On Dec 11, 2015, at 2:01 PM, Todd Fiala via lldb-commits
>>>>> >>> <lldb-commits at lists.llvm.org> wrote:
>>>>> >>>
>>>>> >>>
>>>>> >>>
>>>>> >>> On Fri, Dec 11, 2015 at 1:59 PM, Zachary Turner <
>>>>> zturner at google.com>
>>>>> >>> wrote:
>>>>> >>>>
>>>>> >>>> On Fri, Dec 11, 2015 at 1:55 PM Todd Fiala via lldb-commits
>>>>> >>>> <lldb-commits at lists.llvm.org> wrote:
>>>>> >>>>>
>>>>> >>>>> Hey Eugene and Greg,
>>>>> >>>>>
>>>>> >>>>> I thought we were doing spaces before the open parens in places
>>>>> like
>>>>> >>>>> this:
>>>>> >>>>>
>>>>> >>>>> -    BreakpointResolverSP resolver_sp(new
>>>>> BreakpointResolverFileLine
>>>>> >>>>> (NULL,
>>>>> >>>>> ...
>>>>> >>>>> +    BreakpointResolverSP resolver_sp(new
>>>>> >>>>> BreakpointResolverFileLine(nullptr,
>>>>> >>>>>
>>>>> >>>>> (see the removal of the space after BreakpointResolverFileLine
>>>>> from the
>>>>> >>>>> clang-tidy settings I presume).
>>>>> >>>>>
>>>>> >>>>> Did I misunderstand that?
>>>>> >>>>
>>>>> >>>>
>>>>> >>>> This was officially removed from the coding standard some months
>>>>> ago,
>>>>> >>>
>>>>> >>>
>>>>> >>> Okay.  Are we 100% in sync with LLVM coding standard guidelines?
>>>>> If so I
>>>>> >>> can just look there to see what we're supposed to be doing.
>>>>> >>>
>>>>> >>>
>>>>> >>> No, the differences between the lldb and llvm coding standards are
>>>>> >>> documented in:
>>>>> >>>
>>>>> >>> http://lldb.llvm.org/lldb-coding-conventions.html
>>>>> >>>
>>>>> >>> Jim
>>>>> >>>
>>>>> >>>
>>>>> >>>>
>>>>> >>>> but not everyone has adopted this unfortunately.  See r228860.
>>>>> It pains
>>>>> >>>> me to no end that we differ from LLVM, because it leads to
>>>>> exactly these
>>>>> >>>> type of problems where people aren't sure what the exact set of
>>>>> rules are.
>>>>> >>>
>>>>> >>>
>>>>> >>>
>>>>> >>>
>>>>> >>> --
>>>>> >>> -Todd
>>>>> >>> _______________________________________________
>>>>> >>> lldb-commits mailing list
>>>>> >>> lldb-commits at lists.llvm.org
>>>>> >>> http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
>>>>> >>>
>>>>> >>>
>>>>> >>
>>>>> >>
>>>>> >>
>>>>> >> --
>>>>> >> -Todd
>>>>>
>>>>
>>>
>>>
>>> --
>>> -Todd
>>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/lldb-commits/attachments/20151212/ce10841b/attachment.html>


More information about the lldb-commits mailing list