Page MenuHomePhabricator

[clang-tidy] Adding Fuchsia checker for statically constructed objects
ClosedPublic

Authored by juliehockett on Dec 22 2017, 11:36 AM.

Diff Detail

Event Timeline

juliehockett created this revision.Dec 22 2017, 11:36 AM
Eugene.Zelenko added inline comments.
docs/ReleaseNotes.rst
63

Please highlight constexpr with `, not .

docs/clang-tidy/checks/fuchsia-statically-constructed-objects.rst
7

Please highlight constexpr with `, not .

26

Please highlight constexpr with `, not .

juliehockett marked 3 inline comments as done.

Is the singleton allowed, that uses a static object in the instance method?

What happens for c++98? I realize that fuchsia is c++14 but we might still think about not having constexpr.
If we just assume c++11 you can do the matching only for it. (getLangOpts or similar, see other checks for it.)

aaron.ballman added inline comments.Dec 23 2017, 5:34 AM
clang-tidy/fuchsia/StaticallyConstructedObjectsCheck.cpp
24

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?
26

So things with implicit constructors that are not constexpr should also be caught, or only with explicit non-constexpr constructors?

37

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?

juliehockett marked 3 inline comments as done.
  1. Narrowing check to only warn if the declaration is a global non-trivial object with explicit static storage, unless the object either has a constexpr constructor or the object has no explicit constructor.
  2. Restricting check to only consider C++11 or higher.
  3. Updating tests to reflect the above
aaron.ballman added inline comments.Jan 6 2018, 9:04 AM
clang-tidy/fuchsia/StaticallyConstructedObjectsCheck.cpp
49

Replace the colon with a semicolon.

51

You should drop this part of the string (it should be automatically added by the diagnostics engine).

docs/ReleaseNotes.rst
63

global non-trivial -> global, non-trivial

docs/clang-tidy/checks/fuchsia-statically-constructed-objects.rst
7

global non-trivial -> global, non-trivial

test/clang-tidy/fuchsia-statically-constructed-objects.cpp
27–28

This may be expected behavior, but I find it somewhat strange. This kind of rule is usually in place because you want to prohibit constructors that trigger during program initialization. What is the rule trying to accomplish by disallowing static globals that are declared static while allowing static globals that are not declared with an explicit static keyword?

If the rule is trying to prevent constructors from triggering prior to executing main(), what about constexpr constructors that must be evaluated at runtime? e.g.,

class S {
  int Val;
public:
  constexpr S(int i) : Val(100 / i) {}
  int getVal() const { return Val; }
};

static S s1(1); // Constructor call is constexpr; s1 does not need to be dynamically initialized
static S s2(0); // Constructor call is not constexpr; s2 must be dynamically initialized

extern int get_i();
static S s3(get_i()); // Constructor call is not constexpr; s3 must be dynamically initialized
juliehockett marked 5 inline comments as done.

Updating matcher to catch dynamically initialized constructors and fixing comments.

aaron.ballman accepted this revision.Jan 11 2018, 11:06 AM

Aside from a minor formatting nit that I missed, LGTM!

clang-tidy/fuchsia/StaticallyConstructedObjectsCheck.cpp
52

You can elide the braces here.

This revision is now accepted and ready to land.Jan 11 2018, 11:06 AM
This revision was automatically updated to reflect the committed changes.