[PATCH] D41546: [clang-tidy] Adding Fuchsia checker for statically constructed objects

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: clang-tidy/fuchsia/StaticallyConstructedObjectsCheck.cpp:23
+                  // Match statically stored objects...
+                  hasStaticStorageDuration(),
+                  // ... which have C++ constructors...
----------------
The coding standard document is not very clear about what is and is not covered by this check. For instance, it seems it would also cover `static int i;` (because `i` is an object that is statically constructed).

Do you intend to cover code that has implicit static storage duration, or only if it's explicitly declared with the static storage specifier? For instance, this check currently will flag:
```
struct S { S(); }

static S s1; // Expected
S s2; // Is this expected?
extern S s3; // How about this?
```


================
Comment at: clang-tidy/fuchsia/StaticallyConstructedObjectsCheck.cpp:25
+                  // ... which have C++ constructors...
+                  hasDescendant(cxxConstructExpr(unless(
+                      // ... that are not constexpr.
----------------
So things with implicit constructors that are not constexpr should also be caught, or only with explicit non-constexpr constructors?


================
Comment at: clang-tidy/fuchsia/StaticallyConstructedObjectsCheck.cpp:36
+    diag(D->getLocStart(), "statically constructed objects are disallowed");
+    diag(D->getLocStart(), "if possible, use a constexpr constructor instead",
+         DiagnosticIDs::Note);
----------------
This doesn't seem like a good use of a note (it would make more sense as part of the diagnostic). Also, shouldn't the guidance be to not use the static object in the first place?


https://reviews.llvm.org/D41546