That's correct. I apologize for the confusion.
- Christian
Can we fix this by putting a .clang-format style file in the tests/ folder that disables comment formatting? The sledgehammer approach would be
CommentPragmas: .*
But we can probably do it with a combination of more targeted options too
Hi Christian,
I don't think we will need to modify clang-format to resolve this. At
most we will need to disable the formatting in some regions.
Zachary,
I think we should first look at what kind of comment modifications is
clang-format doing and whether they are preventing us from doing our
stuff. I haven't had time to do that now, but I can look into it more
tomorrow.
pl
I just committed another header cleanup commit, which makes lldb
clang-format-immune ( = it still compiles after a full reformat) on
linux. Other OS's are still likely to have some missed dependencies.
Nice!
However, when I tried running the test suite I got about 150 failures.
Based on a sample of the errors, it looks like the problem is that
clang format messes up the "// Place breakpoint here" annotations we
use in the tests.
Therefore, I propose to apply the clang-format to the lldb source code
only as a first step. After that, as a second step, we can go through
the tests and fix them up so that the comment markers are where we
expect them to be.
Personally, I think that reformatting is most valuable for the debugger code itself. The testsuite following standards seems like a separable issue, and much lower priority.
-Chris
In fact, the lldbinline tests could be completely broken by clang-formatting them. They treat each //% line as a separate command to execute. If clang-format broke those lines, lldbinline tests would stop working.
Sean
Yea, if you see above, I mentioned that clang-format has a style option called CommentPragmas, which allows you to specify a regex for comments that clang-format won’t touch. If you specified CommentPragmas: .* then it would never touch any comment no matter what.
(Note that I haven’t actually tested this or ever used this option in practice, just see that it’s there and claims to be for this use case)
100% agreed, though we do want to avoid multiple formatting passes over time if at all possible. Having exceptions to the comment formatting rules for the entire test suite subtree is acceptable – presuming we aren’t planning to come back and do another pass where we revisit that decision in the near future.
Kate Stone k8stone@apple.com
Xcode Low Level Tools
I actually just submitted the patch. (Sorry, itchy trigger finger or something). r278373. If you have any comments let me know and I’m happy to iterate on it.
Shouldn't this be made general and added to the llvm coding conventions? I was assuming that upon completion of this exercise, we would delete the lldb coding conventions doc.
Jim
I was thinking the same thing too. I figured this was just for the interim.
Chris, did you mean to update the global LLVM style conventions?
I don’t think we can completely get rid of the lldb coding conventions doc; we’ll need this type of thing as long as we use swig:
· enumerations that might end up being in the lldb SB API’s should all be written like:
typedef enum EnumName
{
eEnumNameFirstValue,
eEnumNameSecondValue,
} EnumName;
This redundancy is important because the enumerations that find their way through SWIG into Python will show up as lldb.eEnumNameFirstValue, so including the enum name in the value name disambiguates them in Python.
Some directed questions about this proposal:
-
Will we move to an 80 column limit?
-
Will we move to camel case for variables?
-
Will we stop putting m_ at the front of class ivars and g_ at the front of globals?
-
Will we stop using _sp and _up on the end of shared and unique pointers?
I don’t think we can completely get rid of the lldb coding conventions doc; we’ll need this type of thing as long as we use swig:
· enumerations that might end up being in the lldb SB API's should all be written like:
typedef enum EnumName
{
eEnumNameFirstValue,
eEnumNameSecondValue,
} EnumName;
We still have a document describing how to write code for the SB API's, and comments like that properly belong in the SB API document anyway.
Jim
I was thinking the same thing too. I figured this was just for the interim.
Chris, did you mean to update the global LLVM style conventions?
Yes, I meant that this should get updated:
http://llvm.org/docs/CodingStandards.html#include-style
-Chris
I don’t think we can completely get rid of the lldb coding conventions doc; we’ll need this type of thing as long as we use swig:
· enumerations that might end up being in the lldb SB API's should all be written like:
typedef enum EnumName
{
eEnumNameFirstValue,
eEnumNameSecondValue,
} EnumName;
Space and comment formatting can take place in the API section, but nothing public facing can be renamed in any way. We have an API we have vended, so there will be no naming changes in the API and this extends to the enumerations as well. We will need to ensure nothing bad happens during the formatting.
This redundancy is important because the enumerations that find their way through SWIG into Python will show up as lldb.eEnumNameFirstValue, so including the enum name in the value name disambiguates them in Python.
Some directed questions about this proposal:
- Will we move to an 80 column limit?
Yes that is part of this.
- Will we move to camel case for variables?
Not as part of the first changes.
- Will we stop putting m_ at the front of class ivars and g_ at the front of globals?
I believe these make things much clearer and I would love to see llvm and clang adopt some way to identify member variables. "m_" might be too long, but at least a leading "_" would be nice. I don't think there is anything in the LLVM coding conventions that mentions how to name member variables is there?
- Will we stop using _sp and _up on the end of shared and unique pointers?
Again, this makes things clearer in the code and I would prefer to keep these.
- Will we stop putting m_ at the front of class ivars and g_ at the front of globals?
I believe these make things much clearer and I would love to see llvm and clang adopt some way to identify member variables. “m_” might be too long, but at least a leading “_” would be nice. I don’t think there is anything in the LLVM coding conventions that mentions how to name member variables is there?
Unfortunately there is. The LLVM rule is: everything is camel case. member variables, local variables, global variables, it’s all the same. And that also means you can’t distinguish between members, globals, and locals by looking at them because there’s no difference. I don’t think anyone is particularly fond of that rule, but it is what it is. It’s one of those things where I really wish the LLVM rule was different (and I think everyone else does too), but it’s followed regardless. I’m not opposed to doing something different, but if we do we should minimize that difference as much as possible. Note that _ followed by an uppercase letter are reserved identifiers in C++. So if we’re going to veer from LLVM here, maybe camel case followed by an _ would be a reasonable compromise.
- Will we stop using _sp and _up on the end of shared and unique pointers?
Again, this makes things clearer in the code and I would prefer to keep these.
Personally I’ve never been a fan of the _sp and _up suffixes, and they also kind of clash with camel case naming. So if we move to camel case, then seeing something like Debugger_sp is going to look really weird to me for a variable name. Seems like we should just write DebuggerPtr or something. Whether it’s shared or unique or weak, meh. code completion tools can tell you that kind of stuff I guess.
Just my 2c though, we might be getting ahead of ourselves anyway since we’re not talking about variable renaming yet.
BTW, clang-tidy can find and fix variable naming style issues, so if we decide to do this, there are still tools that can help us.
> - Will we stop putting m_ at the front of class ivars and g_ at the front of globals?
I believe these make things much clearer and I would love to see llvm and clang adopt some way to identify member variables. "m_" might be too long, but at least a leading "_" would be nice. I don't think there is anything in the LLVM coding conventions that mentions how to name member variables is there?
Unfortunately there is. The LLVM rule is: everything is camel case. member variables, local variables, global variables, it's all the same. And that also means you can't distinguish between members, globals, and locals by looking at them because there's no difference. I don't think anyone is particularly fond of that rule, but it is what it is. It's one of those things where I really wish the LLVM rule was different (and I think everyone else does too), but it's followed regardless. I'm not opposed to doing something different, but if we do we should minimize that difference as much as possible. Note that _ followed by an uppercase letter are reserved identifiers in C++. So if we're going to veer from LLVM here, maybe camel case *followed* by an _ would be a reasonable compromise.
I don't really care about global variables, but member variables, I would love to still be able to tell them apart. If we go camel case, the maybe just a leading lower case "m". "m_foo" would become "mFoo"?
> - Will we stop using _sp and _up on the end of shared and unique pointers?
Again, this makes things clearer in the code and I would prefer to keep these.
Personally I've never been a fan of the _sp and _up suffixes, and they also kind of clash with camel case naming. So if we move to camel case, then seeing something like Debugger_sp is going to look really weird to me for a variable name. Seems like we should just write DebuggerPtr or something. Whether it's shared or unique or weak, meh. code completion tools can tell you that kind of stuff I guess.
If we go camel case then debugger_sp would become DebuggerSP.
Just my 2c though, we might be getting ahead of ourselves anyway since we're not talking about variable renaming yet.
Agreed. Lets start with 80 columns and the other formatting stuff to start with.
BTW, clang-tidy can find and fix variable naming style issues, so if we decide to do this, there are still tools that can help us.
Sounds good.
The problem is that’s already the name of the typedef in lldb-forward.h 
Unless we get rid of all those (also not entirely opposed to that TBH).
I recommend approaching this in three steps:
- get the less-controversial changes done that Greg was outlining.
- start a discussion in the llvm community about the concept of a member/global prefix.
2a) the community could agree that llvm-as-a-whole should move to prefixes or otherwise change the camel case policy.
2b) the community could agree that the existing policies are preferred - LLDB moves to whatever is the end result of the discussion.
I guess what I’m saying is that since the opinions about this are very strong, and because we haven’t really had that debate in the LLVM community, that it would be bad to proactively move to the LLVM style, simply to have to move back later. Iff the (sure to be extensive) community discussion settles on the idea that prefixes are the wrong thing, then LLDB should remove them to be consistent.
-Chris
+1
In terms of the formatting of tests, I did some more research on this.
I think the changes needed to be made to the test suite are generally
trivial to fix (e.g. r278490), but I don't think we can avoid a manual
intervention. CommentPragmas does not seem to be a silver bullet -- it
does prevent clang-format from breaking the comment, but it does not
prevent it from moving the whole comment to a new line. That said,
when I reformatted the test sources with CommentPragmas set, the
number of failures went down to 80 (from about 150)...
I believe we should still perform the reformatting of the tests, at
least to standardize on the 2 space indent (in fact we should consider
doing the same for the python code as well, I don't know what's the
situation there in llvm land), but it can be done later. It will make
the period while the code is in flux longer, but hopefully not too
long. Also the modifications will be independent of the main reformat,
so it will still be true that a single source file only got
reformatted once.
pl
I recommend approaching this in three steps:
1) get the less-controversial changes done that Greg was outlining.
2) start a discussion in the llvm community about the concept of a
member/global prefix.
2a) the community could agree that llvm-as-a-whole should move to prefixes
or otherwise change the camel case policy.
2b) the community could agree that the existing policies are preferred
3) LLDB moves to whatever is the end result of the discussion.I guess what I’m saying is that since the opinions about this are very
strong, and because we haven’t really had that debate in the LLVM community,
that it would be bad to proactively move to the LLVM style, simply to have
to move back later. Iff the (sure to be extensive) community discussion
settles on the idea that prefixes are the wrong thing, then LLDB should
remove them to be consistent.-Chris
+1
In terms of the formatting of tests, I did some more research on this.
I think the changes needed to be made to the test suite are generally
trivial to fix (e.g. r278490), but I don't think we can avoid a manual
intervention. CommentPragmas does not seem to be a silver bullet -- it
does prevent clang-format from breaking the comment, but it does not
prevent it from moving the whole comment to a new line. That said,
when I reformatted the test sources with CommentPragmas set, the
number of failures went down to 80 (from about 150)...I believe we should still perform the reformatting of the tests, at
least to standardize on the 2 space indent (in fact we should consider
doing the same for the python code as well, I don't know what's the
situation there in llvm land), but it can be done later. It will make
the period while the code is in flux longer, but hopefully not too
long. Also the modifications will be independent of the main reformat,
so it will still be true that a single source file only got
reformatted once.
My eyes put in a vote for not reformatting the Python to 2 space tabs. In C++, most IDE's do smart things with double-clicking on { to find the closing ones easing the task that two space indents makes somewhat harder. But since the spacing is the only nesting indicator in Python, it would be nice to keep that more visually apparent.