daljit46
commented
9 months ago readability-container-size-empty can be applied using clang-tidy -fix, but the other two would too risky to change using automation. In particular the first would most likely need manual supervision on all instances of const char* as we would need to think about the lifetimes of strings.
I'm in favour of this though, so I think a gradual adoption of std::string and perhaps std::string_view would be ideal. Perhaps, we could start doing this with all application level code (i.e. all code in cmd).
Lestropie
Since #2745,
clang-tidywas introduced. It is run only on proposed changes within PRs, not across the whole code base, since the latter would be overly burdensome. We can however elect to take any recommendations that it makes based on isolated code, and apply the same changes across the code base, eg. in cases where instances of such can be found through a regex search or where it's known that a particular design pattern is used in a finite number of places. This issue will serve to facilitate listingclang-tidyproposals that may be worthwhile revising the code base. Hopefully it will doubly serve to showclang-tidyproposals that we are aware of but may not necessarily resolve in any unrelated PR, so that those less experienced will not panic if they see them.[ ]
cppcoreguidelines-pro-bounds-array-to-pointer-decayExemplified in #2818. Really does not like the way that strings and lists-of-strings are handled in the CLI; particularly forArgument::type_choice(), but also for eg.App::DESCRIPTION. Would be best resolved comprehensively with #2111.[ ]
cppcoreguidelines-macro-usageExemplified in #2818 forcmd/mrview.cpp, but macro functions are likely used in a number of other places.[x]
readability-container-size-empty"warning: the 'empty' method should be used to check for emptiness instead of 'size'" First seen in #2702. Might be most applicable to cases whereget_option_value()should instead be used. The case in #2702 is an enum class, which from memory preventedget_option_value()from being used. But probably also many other instances, and potentially something that could be found via grep. Addressed in #2829.[ ]
readability-uppercase-literal-suffixShown in #2829, though I've seen it elsewhere also. Had never encountered the recommendation that datatype suffices should be capitalised. This is another that might be possible to automatically correct across the code base, at least as a first pass; it's possible that some casts to single precision predate the transition ofdefault_typeto double precision, and therefore in fact the suffix should be removed.[ ]
misc-const-correctnessShown in #2829, but also one I've seen elsewhere. This might be possible to accept automatic changes with some manual revisions to tweak formatting. As long as both major compilers are happy after the change. Only catch if trying to apply an automatic fix here is that it seems to like placing theconstqualifier after the type, rather than before. I presume this is because it better disambiguates cases like const pointers to const objects, but it still looks alien to me currently.[ ]
readability-implicit-bool-conversionThis includes comparing pointers tonullptrand checking if integers are greater than zero. The occurrences I've seen I've agreed with, so might be applicable across the board.[ ]
cppcoreguidelines-init-variablesThere are certainly instances where the content of a variable gets immediately set through some function call, and so the lack of initialization only persists for one line of code. It would nevertheless be preferable to initialise all variables anyway to shut this one up. Indeed in some circumstances I've explicitly usedstd::numeric_limits::signaling_NaN()in initialisers; we may not be catching such, but it clearly signals within the code that it's not intended to be a valid value (unlike other pieces of code where we intentionally exploit NaNs).[ ]
readability-make-member-function-const[ ]
concurrency-mt-unsafeSomewhat related to #2795.[ ]
readability-const-return-typeConverse to prior listing, usingconstwhere it shouldn't be used. Also a potential flag that the return type may have been intended to be a reference but the ampersand was erroneously omitted.