Refactoring code with `cpplint`

Developers
1

Hi!
With the new cpplint, there are some additional defaults, which are currently disabled but I would like to enforce, namely:

  • build/include_order 3301
  • build/header_guard 2805
  • build/namespaces 3
  • runtime/string 36
  • build/include_what_you_use 23

These essentially end causing changes akin to the following

#ifndef STAN_MATH_REV_FUN_VARIANCE_HPP -> #ifndef
STAN_MATH_REV_FUN_VARIANCE_HPP_
#endif -> #endif // STAN_MATH_REV_FUN_VARIANCE_HPP_
For a static/global string constant, use a C style string instead: "const char MAJOR_VERSION[]". [4]
stan/math/rev/functor/cvodes_ode_data.hpp(195): error cpplint:[build/include_what_you_use] Add #include <utility> for move [4]

Since enabling these for the math library will involve making changes in the stan repo, and also will cause much breakage for anyone working with a fork right now, I was hoping to get some opinions about this change.

1 Like
2

Where are these documented at?

Why would changing things at the stan level cause breaking changes in the math repo?

3

Well the linter runs on every C++ file in both repositories, so they have to be changed in tandem. The exact options are documented only via the help string, and pretty vaguely, so I’ll link to sections of the Google C++ Style Guide which cpplint enforces.

4

Are our header tests not catching that everything’s included?

I’m not sure we want to change every one of our header guards just because a style linter decided they should end in “_” this release. We used to have the include order thing that required standard libs last. Is that sitll the include order issue?

5

Sorry for missing this thread. Thanks for listing all these.

I like the comment on the #endif ( #endif // FOO_BAR_BAZ_H_), but I am with Bob on the trailing underscore. Is there a way to only use one of those? Probably not, right?

Does this change anything in our code base? Reading through it quickly, it seems to me that we are following that. Does the 3 next to it indicate that we are missing only three cases? If that is the case I think this one should be added.

We did use the include what you use in the previous cpplint version. Was something added to the rules for the new version that is causing the 23 fails?

These are hopefully happening in the test/ folder? Or do we have some global strings floating around? We do have some global string constants in test/ and I am fine with fixing those (though we should also rethink if we can get rid of the global string).

Bottom line, I think we can safely do the following in one PR

  • build/namespaces 3
  • runtime/string 36
  • build/include_what_you_use 23

I am not sure on the include order, but that is definitely a separate PR with the number of fixes required. But we need to decide on whether we want to do it or not.

6

Thanks for the detailed inputs, I’ll try to address them individually.

I like the comment on the #endif ( #endif // FOO_BAR_BAZ_H_), but I am with Bob on the trailing underscore. Is there a way to only use one of those? Probably not, right?

We can actually modify the linter (it is very simple python) to do that, but in that case we will just have to keep that in mind (documented) so the change is propagated when updating the versions.

I think I can open a PR with

  • build/namespaces 3
  • runtime/string 36
  • build/include_what_you_use 23

in a bit, and add to the discussion on the other rules then.

1 Like
7

That would be fantastic!

I am not sure we want to go that deep. If it was something really really important we might. But the comment on the #endif is, at least in my opinion, not worth the effort and extra maintainance.

1 Like
8

I’m OK with comments on endifs. I don’t really feel they’re necessary since they’re at the end of files and thus always match the header guard. But I can see how it might avoid some mistakes in coding if there were internal ifdefs beyond the outer one.