drakvuf icon indicating copy to clipboard operation
drakvuf copied to clipboard

Adoping a C++ style guide

Open chivay opened this issue 4 years ago • 6 comments

Hi, during its development, DRAKVUF had quite a of of different people working on it often with varying opinions, C++ knowledge and stylistic preferences. This led to inconsistencies between some parts of the code and will always be a a source of disagreement between developers.

My proposal is to adopt Google C++ Style Guide to ensure that with time the codebase will become more uniform and thus easier to understand and develop.

chivay avatar Jan 27 '21 22:01 chivay

This type of standardization is great if we can get contributors to agree. But as a principle I prefer not to enforce it for plugins as I believe people should be allowed to write plugins in any way they feel like. The basic plugin interface and libdrakvuf API will be kept as minimalistic as possible from a C++ perspective for this reason.

More complex C++ layers like plugin_ex and libusermode are strictly opt-in for plugins to use. I largely defer style discussion and C++ feature usage on these to whoever uses them and puts the effort into maintaining it/reviewing changes. If there is a critical disagreement between groups its always possible to fork them and just let everyone use their own preferred wrapper with their preferred style.

So I would rather reframe this standardization topic for these subsystems only. I want to let you guys figure out what standard you want to use for it and enforce going forward. I think it would be valuable and reduce friction in the long term to agree on a subset of C++ features to use. So let's hear from others what they think about it.

/cc @disaykin @skvl

tklengyel avatar Jan 27 '21 23:01 tklengyel

Any auto-mantained code style is fine for me.

disaykin avatar Jan 27 '21 23:01 disaykin

I second that, if we can add a style check to Github Actions for this that would be fantastic.

tklengyel avatar Jan 27 '21 23:01 tklengyel

Although I don't contribute lately I share the point of view from @tklengyel

aoshiken avatar Jan 28 '21 07:01 aoshiken

https://github.com/cpplint/cpplint looks like a solid tool for the job

tklengyel avatar Jan 28 '21 14:01 tklengyel

# cpplint --filter=-whitespace,-legal,-build/header_guard src/libusermode/*
Ignoring src/libusermode/Makefile; not a valid file name (h++, cc, hh, cxx, cuh, hxx, c++, hpp, cu, c, h, cpp)
Done processing src/libusermode/Makefile
Ignoring src/libusermode/Makefile.am; not a valid file name (h++, cc, hh, cxx, cuh, hxx, c++, hpp, cu, c, h, cpp)
Done processing src/libusermode/Makefile.am
Ignoring src/libusermode/Makefile.in; not a valid file name (h++, cc, hh, cxx, cuh, hxx, c++, hpp, cu, c, h, cpp)
Done processing src/libusermode/Makefile.in
Skipping input 'src/libusermode/printers': Can't open for reading
src/libusermode/running.cpp:112:  Found C system header after C++ system header. Should be: running.h, c system, c++ system, other.  [build/include_order] [4]
src/libusermode/running.cpp:113:  Found C system header after C++ system header. Should be: running.h, c system, c++ system, other.  [build/include_order] [4]
src/libusermode/running.cpp:114:  Found C system header after C++ system header. Should be: running.h, c system, c++ system, other.  [build/include_order] [4]
src/libusermode/running.cpp:115:  Found C system header after C++ system header. Should be: running.h, c system, c++ system, other.  [build/include_order] [4]
src/libusermode/running.cpp:116:  Found C system header after C++ system header. Should be: running.h, c system, c++ system, other.  [build/include_order] [4]
src/libusermode/running.cpp:117:  Found C system header after C++ system header. Should be: running.h, c system, c++ system, other.  [build/include_order] [4]
src/libusermode/running.cpp:118:  Found C system header after C++ system header. Should be: running.h, c system, c++ system, other.  [build/include_order] [4]
Done processing src/libusermode/running.cpp
src/libusermode/uh-private.hpp:111:  Found C system header after C++ system header. Should be: uh-private.h, c system, c++ system, other.  [build/include_order] [4]
src/libusermode/uh-private.hpp:169:  Using C-style cast.  Use reinterpret_cast<rh_data_t*>(...) instead  [readability/casting] [4]
src/libusermode/uh-private.hpp:234:  Single-parameter constructors should be marked explicit.  [runtime/explicit] [5]
src/libusermode/uh-private.hpp:215:  Add #include <map> for map<>  [build/include_what_you_use] [4]
src/libusermode/uh-private.hpp:226:  Add #include <string> for string  [build/include_what_you_use] [4]
Done processing src/libusermode/uh-private.hpp
src/libusermode/userhook.cpp:123:  Found C system header after C++ system header. Should be: userhook.h, c system, c++ system, other.  [build/include_order] [4]
src/libusermode/userhook.cpp:124:  Found C system header after C++ system header. Should be: userhook.h, c system, c++ system, other.  [build/include_order] [4]
src/libusermode/userhook.cpp:125:  Found C system header after C++ system header. Should be: userhook.h, c system, c++ system, other.  [build/include_order] [4]
src/libusermode/userhook.cpp:126:  Found C system header after C++ system header. Should be: userhook.h, c system, c++ system, other.  [build/include_order] [4]
src/libusermode/userhook.cpp:127:  Found C system header after C++ system header. Should be: userhook.h, c system, c++ system, other.  [build/include_order] [4]
src/libusermode/userhook.cpp:128:  Found C system header after C++ system header. Should be: userhook.h, c system, c++ system, other.  [build/include_order] [4]
src/libusermode/userhook.cpp:129:  Found C system header after C++ system header. Should be: userhook.h, c system, c++ system, other.  [build/include_order] [4]
src/libusermode/userhook.cpp:185:  Use int16/int64/etc, rather than the C type long  [runtime/int] [4]
src/libusermode/userhook.cpp:214:  Use int16/int64/etc, rather than the C type long  [runtime/int] [4]
src/libusermode/userhook.cpp:317:  Use int16/int64/etc, rather than the C type long  [runtime/int] [4]
src/libusermode/userhook.cpp:326:  Use int16/int64/etc, rather than the C type long  [runtime/int] [4]
src/libusermode/userhook.cpp:331:  Use int16/int64/etc, rather than the C type long  [runtime/int] [4]
src/libusermode/userhook.cpp:403:  Use int16/int64/etc, rather than the C type long  [runtime/int] [4]
src/libusermode/userhook.cpp:404:  Use int16/int64/etc, rather than the C type long  [runtime/int] [4]
src/libusermode/userhook.cpp:717:  Use int16/int64/etc, rather than the C type long  [runtime/int] [4]
src/libusermode/userhook.cpp:718:  Use int16/int64/etc, rather than the C type long  [runtime/int] [4]
src/libusermode/userhook.cpp:865:  Add #include <utility> for move  [build/include_what_you_use] [4]
Done processing src/libusermode/userhook.cpp
src/libusermode/userhook.hpp:111:  Found C system header after C++ system header. Should be: userhook.h, c system, c++ system, other.  [build/include_order] [4]
src/libusermode/userhook.hpp:166:  Add #include <utility> for move  [build/include_what_you_use] [4]
src/libusermode/userhook.hpp:273:  Add #include <string> for string  [build/include_what_you_use] [4]
Done processing src/libusermode/userhook.hpp
Total errors found: 32

tklengyel avatar Jan 28 '21 16:01 tklengyel