Upgrade clang-format and clang-tidy to v14 #6891
Conversation
| *semaphore_t_type; | ||
| llvm::Type *void_t = nullptr, *i1_t = nullptr, *i8_t = nullptr, *i16_t = nullptr, *i32_t = nullptr, *i64_t = nullptr, *f16_t = nullptr, *f32_t = nullptr, *f64_t = nullptr; | ||
| llvm::StructType *halide_buffer_t_type = nullptr, | ||
| *type_t_type, |
There was a problem hiding this comment.
I think it's currently benign, but it would be good to initialize all of these to null instead of just some of them.
| const bool is_buffer; | ||
| MemoryType memory_type = MemoryType::Auto; | ||
|
|
||
| ParameterContents(Type t, bool b, int d, const std::string &n) |
There was a problem hiding this comment.
Reading through this PR, I'm not so sure this new check is a good thing. It splits up initialization of fields by a not-very-useful criterion so that you have to look in two places to discover what the initial values are for the members.
| template<typename ElemType, int BUFFER_SIZE = 1024> | ||
| struct ElemWriter { | ||
| ElemWriter(FileOpener *f) | ||
| : f(f), next(&buf[0]), ok(true) { |
There was a problem hiding this comment.
Here for example I think it's much clearer if the initial state for ElemWriter is written down in a single place, instead of being split between the constructor and the members.
There was a problem hiding this comment.
Interesting! I see it a different way: with this change, all of the members that are default-inited (either to a constant value or default-ctor for nontrivial types) are done so in the class declaration, leaving the classical "ctor-init-list" used only for things that are inited by an argument to the ctor itself (or by expressions that aren't otherwise constant). IMHO that is more clear.
There was a problem hiding this comment.
This particular case was what tipped me over the edge to not liking it to the point of commenting. next(&buf[0]) is not a function of the arguments, it's just the initial value for that field. I don't see it as relevant that those bits aren't a compile-time constant.
There was a problem hiding this comment.
I think this case is probably a limitation of clang-tidy being conservative (ie, being unable to prove to itself that this was a valid construct) -- ElemType *next = &buf[0]; is perfectly valid here AFAICT (and I'll push a manual edit to this PR to fix that).
That said, this isn't an issue I want to spend any nontrivial amount of time on; I'll just open a new PR with the new clang-tidy behavior disabled and see how that looks. (The main desire here is just to keep our tooling relatively new, not to enforce arbitrary new checks.)
|
|
||
| StmtToHtml(const string &filename) | ||
| : id_count(0), context_stack(1, 0) { | ||
| : context_stack(1, 0) { |
There was a problem hiding this comment.
Here's another case where it doesn't depend on the args - it's just not the sort of constant that's blessed with member initialization
| char snam; | ||
| std::string desc; | ||
| bool has; | ||
| bool has = false; |
There was a problem hiding this comment.
I also find it weird to read a list of members like this that vary between initialized and not, depending on whether their initial value is a compile-time constant or not.
There was a problem hiding this comment.
This particular example isn't a good one for your point, IMHO (the strings were always initialized 'here' and not in the ctor, and snam was left uninitialized, period)
That said, I see your point here -- it is unfortunate that C++ has multiple ways to do this, often located in separate files, and so regardless of which convention we use here, we can't know for sure whether (and how) snam is initialized at all.
I'm not married to this change, but I must admit that I do prefer the idea (and readability) of member vars being explicitly initialized at the declaration point -- it strikes me as putting more critical information about a members initial value along with its name and type. Unless you are violently allergic to this, though, why don't I leave it open here for a while and see if anyone else has thoughts pro or con. (And we can easily disable modernize-use-default-member-init entirely and revert those parts of the change, so if opinion is a "nay", I still think upgrading the tool versions is worthwhile)
(It would be interesting to imagine a world in which C++ required all variables to be explicitly initialized... with the ability to explicitly leave a field uninitialized via = poison; or something similar.)
|
Please see #6902 instead. |
With LLVM16 being branched off, that means we'll be dropping LLVM13 in the months ahead; this PR proposes to upgrade our required versions of clang-tidy and clang-format to those for LLVM14 (so that no one will have to build or install LLVM13 just for those).
The changes for clang-format are insignificant (it appears to be a bit more aggressive about fixing single-line if statements, which is good).
The changes for clang-tidy include an update to the
modernize-use-default-member-initwhich I think is arguably a good thing, so I elected to leave it enabled and have clang-tidy fix it. Basically, it prefers that member variables that are inited to constant values do so using themember = value;style, rather than the oldctor() : member(value){}style, e.g.