This checks finds all primitive type local variables (integers, doubles, pointers) that are declared without an initial value. Includes fixit functionality to initialize said variables with a default value. This is zero for most types and NaN for floating point types. The use of NaNs is copied from the D programming language.
Details
Diff Detail
Event Timeline
This certainly needs more tests: macros, -x C, ???
clang-tools-extra/clang-tidy/misc/InitLocalVariablesCheck.cpp | ||
---|---|---|
26–29 ↗ | (On Diff #209609) | Can make these proper AST_MATCHER and do this in registerMatchers(). |
32–35 ↗ | (On Diff #209609) | This feels brittle. |
41–48 ↗ | (On Diff #209609) | should be var = <init>; |
43 ↗ | (On Diff #209609) | I'm pretty sure there's simple utility function that can insert include. |
clang-tools-extra/clang-tidy/misc/InitLocalVariablesCheck.cpp | ||
---|---|---|
8 ↗ | (On Diff #209609) | Please separate header from include files with empty line. |
31 ↗ | (On Diff #209609) | Please don't use auto when type could not be deduced from same statement. |
41 ↗ | (On Diff #209609) | May be spaces should be created around =? Same in other places. |
56 ↗ | (On Diff #209609) | Please run Clang-format. |
clang-tools-extra/clang-tidy/misc/InitLocalVariablesCheck.h | ||
28 ↗ | (On Diff #209609) | Unnecessary empty line. |
clang-tools-extra/docs/clang-tidy/checks/misc-init-local-variables.rst | ||
6 ↗ | (On Diff #209609) | Please synchronize first statement with description in Release Notes. |
31 ↗ | (On Diff #209609) | Spaces around =. Same in other places. |
clang-tools-extra/clang-tidy/misc/InitLocalVariablesCheck.cpp | ||
---|---|---|
26–29 ↗ | (On Diff #209609) | Init checking is now in the matcher. I did not change isLocalVarDecl because according to the documentation there does not seem to be a builtin matcher for that. Since the isLocalVarDecl is semantically exactly what is needed here it seems a bit silly to reimplement that from scratch from matcher basic blocks. |
32–35 ↗ | (On Diff #209609) | There's not much else one can do. Also, all variable names that begin with an underscore are reserved for the standard library (I think), so we should not be changing those in any case. |
clang-tools-extra/clang-tidy/misc/InitLocalVariablesCheck.cpp | ||
---|---|---|
21 ↗ | (On Diff #209706) | I believe, this should skip matches within template instantiations. Consider this code: template<typename T> void f(T) { T t; } void g() { f(0); f(0.0); } What will the fix be? |
32 ↗ | (On Diff #209706) | Should this just disallow all fixes within macros? Maybe warnings as well. |
clang-tools-extra/clang-tidy/misc/InitLocalVariablesCheck.cpp | ||
---|---|---|
21 ↗ | (On Diff #209706) | I tested with the following function: template<typename T> void template_test_function() { T t; int uninitialized; } Currently it warns on the "uninitialized" variable regardless of whether the template is instantiated or not. If you call it with an int type, it will warn about variable t being uninitialized. If you call it with a, say, struct type, there is no warnings. Is this a reasonable approach? |
32 ↗ | (On Diff #209706) | I can change that, seems reasonable. Should it still retain this check, though? One would imagine there are other ways of getting variables whose names begin with an underscore. |
clang-tools-extra/clang-tidy/misc/InitLocalVariablesCheck.cpp | ||
---|---|---|
21 ↗ | (On Diff #209706) | And what happens, if there are multiple instantiations of the same template, each of them requiring a different fix? Can you try the check with my example above (and maybe also add f("");inside g()). I believe, the check will produce multiple warnings with conflicting fixes (and each of them will be wrong, btw). |
32 ↗ | (On Diff #209706) | I don't know, whether the check for leading underscore will still be valuable. I don't think there's a valid reason why variables with a leading underscore in their names should in general not be initialized. |
clang-tools-extra/clang-tidy/misc/InitLocalVariablesCheck.cpp | ||
---|---|---|
21 ↗ | (On Diff #209706) | Interestingly it does warn about it, but only once, even if you have two different template specializations. I tried to suppress this warning when the type being instantiated is a template argument type but no matter what I tried I could not get this to work. Is there a way to get this information from the MatchedDecl object or does one need to do something more complicated like going up the AST until a function definition is found and checking if it is a template specialization (presumably with TemplatedKind)? Any help would be appreciated. |
clang-tools-extra/clang-tidy/misc/InitLocalVariablesCheck.cpp | ||
---|---|---|
21 ↗ | (On Diff #209706) | If there are multiple warnings with the same message at the same location (clang-tidy/ClangTidyDiagnosticConsumer.cpp:745), they will be deduplicated. Thus, a random fix will probably be suggested. The proper way to filter out matches in template instantiations is to add unless(isInTemplateInstantiation()) to the matcher. |
clang-tools-extra/clang-tidy/misc/InitLocalVariablesCheck.cpp | ||
---|---|---|
21 ↗ | (On Diff #209706) | I tried to make this work but I just could not combine statement and declaration matching in a reliable way. Matching a statement that is not in a template declaration can be done, as well as matching a declaration without intial value, but combining those two into one is hard. After trying many, many things the best I could come up with was this: declStmt(containsDeclaration(0, varDecl(unless(hasInitializer(anything()))).bind("vardecl"))), this) The problem is that containsDeclaration takes an integer denoting how manyth declaration should be processed. Manually adding matchers for, say, 0, 1, 2, 3 and 4 works and does the right thing but fails if anyone has an uninitialized variable in the sixth location, things will silently fail. The weird thing is that if you do the matching this way, you don't need to filter out things with unless(isInTemplateInstantiation()). Maybe statements are handled differently from declarations? |
clang-tools-extra/clang-tidy/misc/InitLocalVariablesCheck.cpp | ||
---|---|---|
21 ↗ | (On Diff #209706) | I was struggling to understand, why you want to match a statement, but then I figured out that I should have been more precise: while isInTemplateInstantiation only works for Stmts, there's a related matcher that works for Decls: isInstantiated. See clang/include/clang/ASTMatchers/ASTMatchers.h:5187. In general, looking into this header can be useful, if you want to find a matcher that you can vaguely describe (e.g. when looking for something related to instantiations, you can search for the relevant substring and find this and a bunch of other matchers). Sorry for the confusion. I hope, the suggestion helps. |
A general comment: "misc" is a sort of a heap of checks that otherwise don't have a good home. This one would probably better go to bugprone (or maybe there's a relevant CERT or C++ Core Guidelines rule?).
The closest I could find is ES.20 "Always initialize an object": https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#es20-always-initialize-an-object
Most other guidelines seem to be formulated as "don't read uninitialized memory" such as: https://cwe.mitre.org/data/definitions/457.html
So a potential name for this could be "cppcoreguidelines-init-variables". However that rule is about initializing all variables, even those in structs and classes. This is about local variables.
Related to this, there seems to be a talk at CppCon about doing something similar to this MR: https://cppcon2019.sched.com/event/SfYc/killing-uninitialized-memory-prot
I am in favor of something like cppcoreguidelines-init-variables that might catch other cases in the future as well. IMHO its fine to start with something partial, especially "just" local variables.
clang-tools-extra/test/clang-tidy/cppcoreguidelines-init-variables.cpp | ||
---|---|---|
4 | It'll be better to include cstdint. |
clang-tools-extra/test/clang-tidy/cppcoreguidelines-init-variables.cpp | ||
---|---|---|
4 | I'll correct that comment: tests normally shouldn't depend on system headers; can you just mock whatever you need from there? |
clang-tools-extra/test/clang-tidy/cppcoreguidelines-init-variables.cpp | ||
---|---|---|
4 | If test is C++, why not to include C++ equivalent of system headers? See modernize-deprecated-headers. |
clang-tools-extra/test/clang-tidy/cppcoreguidelines-init-variables.cpp | ||
---|---|---|
4 | Because we want our tests to be hermetic and rely on the system running the test as little as possible. |
clang-tools-extra/test/clang-tidy/cppcoreguidelines-init-variables.cpp | ||
---|---|---|
4 | I'm not commenting on c-vs-c++ header, i'm specifically commenting on an <> include; unless i'm misremembering this is advised against. |
I used stdint to replicate a real world use case as I'd imagine those types would match this search quite heavily.
The tests already have one test for a typedeffed integer and one that is defined with a macro. If those are deemed sufficient, the stdint type can be removed altogether. If the stdint types are defined via some other mechanism such as compiler intrinsics, there would be no test for that. I do not know if that is the case with current libcs.
I can update the MR as requested once someone with enough stripes makes the policy decision.
The problem is that this uses the system header search paths and so the testing system has no control over *what* is found from bot to bot. That is why we structure almost all of our tests to not use system includes.
The tests already have one test for a typedeffed integer and one that is defined with a macro. If those are deemed sufficient, the stdint type can be removed altogether. If the stdint types are defined via some other mechanism such as compiler intrinsics, there would be no test for that. I do not know if that is the case with current libcs.
I don't see a need for int32_t specifically in this test.
I can update the MR as requested once someone with enough stripes makes the policy decision.
I'm not certain that we've written this instruction down so much as let new contributors know when they stumble into it. It would probably be a good idea for us to get some testing documentation written down at some point. Despite that, our policy has been to not use #include in test cases unless the included file is relative to the test.
Updated patch to remove #include<stdint.h> and the corresponding uint32_t test code snippet.
clang-tools-extra/clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp | ||
---|---|---|
46 | Please keep this list sorted alphabetically by string literal. | |
clang-tools-extra/clang-tidy/cppcoreguidelines/InitVariablesCheck.cpp | ||
22 | This should not match on variable declarations that are automatically zero-initialized or have a default constructor, no? | |
30 | You can move this closer to where it's being used. | |
32–33 | I think this should be made into a local AST matcher so the check can be hoisted into the matchers. | |
54 | No need for this local; you can lower into its only use. | |
57 | You should elide braces for any single-statement blocks per our coding conventions. | |
60 | I would rather see the correct NaN inserted along with the include. See StringFindStartswithCheck.cpp for an example of how to use the include inserter. | |
61 | Do you expect this check to be run over ObjC code? If so, this should probably be isAnyPointerType(). | |
62 | This should be checking for C++11. |
Updated to fix review comments. NOTE: detecting the include fix is broken because I could not get it to work.
clang-tools-extra/clang-tidy/cppcoreguidelines/InitVariablesCheck.cpp | ||
---|---|---|
22 | That is correct. The test file has checks for global variables and struct members without initialisation. | |
32–33 | As was discussed earlier, the use of isLocalVarDecl is used here because:
If there is a way to use that function directly in the matcher code, let me know how it's done and I will fix this. But if not, then sadly I have no idea how that should be fixed and it is probably a bigger development task than this checker itself. | |
60 | I copied the implementation and could make it add the update. However I could not for the life of me make the test framework accept this. It will always flag an error even though there should be a comment that declares it in the test source. Any help is appreciated. |
clang-tools-extra/clang-tidy/cppcoreguidelines/InitVariablesCheck.cpp | ||
---|---|---|
32–33 | You can create your own AST matchers that are used only in this translation unit. e.g., AST_MATCHER(VarDecl, isLocalVarDecl) { return Node.isLocalVarDecl(); } and then use this in your AST matchers. | |
60 | So the include is inserted as you expect, but the test continues to fail? What does your test look like for it? What failures are you getting? | |
77 | In C++11 mode, I think this should recommend std::numeric_limits<type>::quiet_NaN() if possible, from <limits> rather than <math.h>. | |
87–102 | I think you want this to be: auto Diagnostic = diag(MatchedDecl->getLocation(), "variable %0 is not initialized") << MatchedDecl << FixItHint::CreateInsertion( MatchedDecl->getLocation().getLocWithOffset( MatchedDecl->getName().size()), InitializationString); if (AddMathInclude) { auto IncludeHint = IncludeInserter->CreateIncludeInsertion( Source.getFileID(MatchedDecl->getBeginLoc()), MathHeader, false); if (IncludeHint) Diagnostic << *IncludeHint; } |
clang-tools-extra/clang-tidy/cppcoreguidelines/InitVariablesCheck.cpp | ||
---|---|---|
32–33 | Fixed, thanks. | |
60 | Just the test in this patch. However after updating the diagnostic code to the one recommended made this problem go away. This now works as intended. | |
77 | As a stylistic point I would disagree with this. In this particular case the "C++ way" looks very verbose and confusing. For a single variable this is not a huge problem, but it gets very ugly when you have something like this (which is common in real world code bases): double d1, d2, d3, d4, d5, d6, d7; This just explodes and is very ugly and unpleasant to read when every one of these gets the std::numeric_limits template value. I would recommend using plain NAN for terseness and readability, but can update the PR if the template form is deemed the better choice. |
clang-tools-extra/clang-tidy/cppcoreguidelines/InitVariablesCheck.cpp | ||
---|---|---|
67 | Elide braces. | |
77 | Let's go with NAN for now. We can always add an option letting the user pick which style to replace with later. | |
clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines-init-variables.rst | ||
34 | This looks incorrect. | |
clang-tools-extra/test/clang-tidy/cppcoreguidelines-init-variables.cpp | ||
4 | You can drop this and its uses, we don't care about warnings produced by Clang about the variables not being used. | |
37 | I'd like to see a test using a local static (which should not diagnose, as it's zero-initialized by default) and a local extern (which should not diagnose because it cannot have an initializer). | |
56 | I'd like to see some tests for other initialization forms as well: int braces{42}; int parens(42); |
Aside from the missing documentation bit, I think this LG.
clang-tools-extra/clang-tidy/cppcoreguidelines/InitVariablesCheck.cpp | ||
---|---|---|
30 | You need to document this option. |
Added documentation. Well, actually just swiped it from abseil-string-find-startswith.
It'll be reasonable to get IncludeStyle default from .clang-format.
clang-tools-extra/docs/ReleaseNotes.rst | ||
---|---|---|
71 | Please sort new checks list alphabetically. |
It'll be reasonable to get IncludeStyle default from .clang-format.
I looked at existing checks and they all do the same thing as this one. In fact I got the code for this by directly copypasting an existing check.
Grepping for clang format in the check tree produces zero relevant matches, so it's unclear to me how this should be written (assuming that this is not taken care of automatically by utility code).
clang-tools-extra/docs/ReleaseNotes.rst | ||
---|---|---|
71 | The list is already incorrectly alphabetized. Should I reorganize all the entries or just put mine in a "correct" spot (between "bugprone" and "linuxkernel")? |
SInse this option may be also set in .clang-format, it's reasonable to re-use it instead of forcing user to duplicate it.
clang-tools-extra/docs/ReleaseNotes.rst | ||
---|---|---|
71 | It'll not hurt to correct other mistakes too. |
clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines-init-variables.rst | ||
---|---|---|
41 | This is still missing the documentation for MathHeader and its default value. |
Do you need someone to commit this on your behalf (sorry for not asking that question sooner)?
Yes, please. I have no rights of any kind, this is in fact my first ever pull request to LLVM.
I'm happy to commit for you, but applying the patch causes merge conflicts. Can you rebase on ToT?
Oh! I just noticed you made the patch against the monorepo which I'm not using (I'm still on svn), so the paths in your patch file are all wrong for me. After some surgery on the paths, it applied cleanly for me.
I committed on your behalf in r373489. Thank you for the new feature!
Please keep this list sorted alphabetically by string literal.