[WIP] Test clang-format#34
Conversation
jpivarski
left a comment
jpivarski
left a comment
There was a problem hiding this comment.
Okay, most of these are indentation and formatting things; I'm not seeing the value that clang-tidy brings. As you can see in the line-by-line comments, some of them are nice, but they're not good enough to go through the exercise of finding just the options we (I?) want.
I'd rather just not use it. Is there a different way I can help you get an overall view of the codebase? If we can't think of anything, maybe I should come up with a small, beginning project.
Maybe implementing a flatten method. This method does a simple thing but it would have to touch a cross-section of the entire codebase, which could be a good way to gain familiarity. If you're interested in this, start a new [WIP] pull request and I'll describe it there (to keep all of that information in one place).
| @@ -1,77 +1,107 @@ | |||
| // BSD 3-Clause License; see https://github.com/jpivarski/awkward-1.0/blob/master/LICENSE | |||
| // BSD 3-Clause License; see | |||
| // https://github.com/jpivarski/awkward-1.0/blob/master/LICENSE | |||
There was a problem hiding this comment.
If this is just a linewidth thing, I'd skip this.
| #include "awkward/Slice.h" | ||
| #include "awkward/cpu-kernels/util.h" | ||
| #include "awkward/io/json.h" | ||
| #include "awkward/type/Type.h" |
There was a problem hiding this comment.
It looks like it's alphabetizing the includes. I used to do that my hand in Emacs because I could easily pipe it out to a shell command.
It would be more interesting if it could catch unused headers, which I probably have. In Python, I use fully qualified names everywhere, so I can easily check for unused imports by string-search. It's a much harder problem with C++ includes.
Removing unused headers could make the builds a little faster, which are a little uncomfortable at up to 2 minutes, since that impedes my favorite way of debugging.
| public: | ||
| Content(std::shared_ptr<Identity> id, std::shared_ptr<Type> type) | ||
| : id_(id), type_(type) {} | ||
| virtual ~Content() {} |
There was a problem hiding this comment.
This indentation thing: let's turn it off. I'm using the indentation that comes with Atom, and Emacs seems to agree with it.
There's a style right now and it's consistent and motivated by the editors we use. (Do you have a favorite editor? Will it work with the indentation we have now?) This is an example of something I don't want to change.
| static Ref newref(); | ||
| static std::shared_ptr<Identity> none() { | ||
| return std::shared_ptr<Identity>(nullptr); | ||
| } |
There was a problem hiding this comment.
Making all inline functions into multi-line functions is fine. It's a natural, readable way to reduce line length.
| Identity(const Ref ref, const FieldLoc fieldloc, int64_t offset, | ||
| int64_t width, int64_t length) | ||
| : ref_(ref), fieldloc_(fieldloc), offset_(offset), width_(width), | ||
| length_(length) {} |
There was a problem hiding this comment.
But not this. I made all those parameters line up for a reason. Some of these classes have long parameter lists (on purpose) and formatting them as a vertical line makes them easier to read.
|
|
||
| typedef IdentityOf<int32_t> Identity32; | ||
| typedef IdentityOf<int64_t> Identity64; | ||
| } // namespace awkward |
There was a problem hiding this comment.
That's cute. I think I like that end-of-namespace signifier, so that you know that it's not the end of a function or class.
It doesn't do much, though, because each of these files is one big namespace (except util).
|
|
||
| const std::string tostring_part(const std::string indent, | ||
| const std::string pre, | ||
| const std::string post) const; |
There was a problem hiding this comment.
These kinds of linewidth reductions aren't needed because const std::string indent, const std::string pre, const string post is a common pattern.
Now here's something that I wish it had caught: I would have liked those to be passing constant references so that it doesn't copy strings. It doesn't matter all that much for performance (our performance efforts would be concentrated on the cpu-kernels in the deepest layer of the architecture), but I'd like to be consistent.
I've been really inconsistent about how I pass references to heavyweight (non-numeric) objects: I'd like it to be const ObjectType& objectvalue, but I sometimes forget the & and I sometimes forget the const. I very rarely mutate objects in place and most of the methods don't change their class, so there should be const almost everywhere. (I wish C++ opted into mutability instead of opting into immutability, but oh well.)
There was a problem hiding this comment.
By the way, my statement of "I would have liked" doesn't mean that you need to do that sweep. Either one of us but probably me will do it and it won't take much more than an hour. I'm just saying that a more useful warning would be one that catches the lack of const-references, rather than the indentation.
| virtual const std::shared_ptr<SliceItem> shallow_copy() const = 0; | ||
| virtual const std::string tostring() const = 0; | ||
| virtual bool preserves_type(const std::shared_ptr<Type> &type, | ||
| const Index64 &advanced) const = 0; |
There was a problem hiding this comment.
This kind of linewidth-sculpting makes it less clear. Maybe we just want a larger linewidth. GitHub has a natural linewidth and we're often viewing this code in GitHub, so maybe that should be the linewidth.
(Incidentally, I determined the maximum width of the string-view of awkward1.Array objects (in Python) to be such that it fits into GitHub and StackOverflow text boxes without scrolling. That's what's really important!)
| const Index64 &advanced) const; | ||
| virtual const std::shared_ptr<Content> | ||
| getitem_next(const SliceFields &fields, const Slice &tail, | ||
| const Index64 &advanced) const; |
There was a problem hiding this comment.
The wide form of these was easier to read because it packs similar function names and arguments into something that can be vertically scanned. And this one goes wider than the GitHub window.
Okay, maybe no hard linewidth-boundary at all.
I thought this program was going to raise alerts about things it finds problematic and we can decide if we want to pay attention to those alerts on a case-by-case basis. If it automatically changes it, I don't think I like it.
|
yes, I agree. |
fix for running standalone HCal, otherwise, the code always looks for ECal
2 participants
C and C++ files automatically formatted with clang-format code. Please, see
https://electronjs.org/docs/development/clang-format for more details.
@jpivarski - you can also integrate clang-format directly into Atom.