poco icon indicating copy to clipboard operation
poco copied to clipboard

Use clang-tidy in project

Open teksturi opened this issue 1 year ago • 5 comments

It would be nice to get clang-tidy to use. It has lot of good rules which makes code base modern and more unified.

Releated items:

  • #4348
  • #4352
  • https://github.com/pocoproject/poco/pull/4333#discussion_r1427425635

Task list

  • [ ] Separate external source files to own folder

NOTE: Reorganisation moved to #4358.

It would be quite hard to exclude every external source without moving them to different folder. This same thing will happened also with other static analyzer tools, formatting tools, compiler options. So if we move them separated folders we can always exclude them more easily.

We can consider couple options here (example for zlib). Note that "external" can also be replaced with some other word if wanted.

  1. Foundation/external/
  2. Foundation/external/zlib/
  3. Foundation/src/external/
  4. Foundation/src/external/zlib/
  5. Foundation/src/zlib
  6. external/
  7. external/zlib

My personal vote goes to 2, 4 or 7. Probably 7 because that way it so easy to seperete everything to one place. We can have example update script which checks all new version for those.

Still need to think what about external files which are also in include/ there we would probably need external folder. Example of this kind of file is "Foundation/include/Poco/ordered_map.h"

  • [ ] Create .clang-tidy config file

This should probably have at least two different sections. One which have WarningsAsErrors true and one with false. This way we can separate what we always want to be clean. I belive there can be sections sepereted by "---"

  • [ ] Add CMake targets tidy-fix and tidy-warn

    These will be excluded from target "all". Maybe also excluded if project is not root project. So external projects will not see this target.

  • [ ] Add CI step

    CI step will run tidy-fix and tidy-warn separately. Even if other fails other step will be run so we catch all at once. Note that even tidy-fix will not fix those to PR. It will just show what could be automatically fixed.

  • [ ] Take use chosen clang-tidy checks

See full list from https://clang.llvm.org/extra/clang-tidy/checks/list.html

I made public google sheet for different checks. It is not complete yet and it might never be but that is good start what we should start using. Note that it is currently my suggestion. Anyone has access right to it.

https://docs.google.com/spreadsheets/d/113ft-sPocWy9AK4uoPloZ9NXISxSWseizM65vj7dYLw/edit?usp=sharing

teksturi avatar Dec 17 '23 17:12 teksturi

Maybe something like this for external files.

image

I still need to prototype a little bit. We probably still do not want to build these as libs as before. Also we probably want to have nice include paths

#include "Poco/external/pdjson/pdjson.h"

We could also do some magic that we can always do that even when POCO_UNBUNDLED is used. But that is not very high priority but would be nice.

teksturi avatar Dec 17 '23 22:12 teksturi

I propose to split the structure reorganisation of external files as an enhancement to a separate issue, because it can be done independently from using clang-tidy.

Regarding includes: AFAIK Poco uses the includes only internally and it does not export them for Poco users. My proposal for includes is that an include path is set for each bundled library in such a way that it can be used in the same way as for unbundled libraries from Poco sources.

matejk avatar Dec 18 '23 07:12 matejk

Structure reorganization is needed if we want to have clang-tidy automated in good way. Else we will clatter with exclude rules. That would be same work as moving files to different location. Moving them will also help in other stuff like taking clang-format in use.

Easiest reorganization would be this

Foundation/src/External/
Foundation/include/Poco/External/

This way almost nothing change but we can exclude those pretty easily.

teksturi avatar Dec 18 '23 08:12 teksturi

I know that it is needed. However restructuring can be done independently of clang-tidy as a separate task. Then clang-tidy can be added on top.

matejk avatar Dec 18 '23 13:12 matejk

You are free to do seperete task for it and edit this one.

teksturi avatar Dec 18 '23 14:12 teksturi