harmonize the include orders in platform2 and related projects |
||
Issue descriptionCurrently, in src/platform2/CPPLINT.cfg, the order of the include files are restricted to alphabetical only, however, google style (https://google.github.io/styleguide/cppguide.html#Names_and_Order_of_Includes) indicates the order of include files should be: Related header, double-quote-include C library, bracket-include C++ library, bracket-include other libraries' .h, double-quote-include your project's .h. double-quote-include But this is totally ignored in platform2 projects. All google3 and chromium projects follow this style (e.g. https://groups.google.com/a/chromium.org/forum/?pli=1#!searchin/chromium-dev/cros$20lint/chromium-dev/Ri_nOYyD9no/UvmvhN6CAAAJ). As a result we have things like src/platform2/touch_keyboard/uinputdevice.h:8 : #include <base/logging.h> #include <error.h> #include <fcntl.h> #include <gtest/gtest_prod.h> // for FRIEND_TEST #include <linux/uinput.h> #include <stdio.h> #include <string> #include <string.h> #include <unistd.h> As google3 and chromium have only one repository and source files directly have access to other libraries. In chrome os this is not exactly the case because "other libraries" include files are exported to system folders. So it might make sense to keep the current style. But I don't think there is anything that prevents us from migrating platfrom2 projects since we can add -I flags to builds. vapier@: Do you think it would worth gradually invest in fixing this issue? I can write some scripts to make the manual labor easier. clang-format can also help arrange stuff. What do you think?
,
Mar 14 2018
By "the order of the include files are restricted to alphabetical only", you mean within each section, right? There are many files in platform2 that follow the Google style guide's ordering rules and alphabetize within sections; see e.g. https://chromium.googlesource.com/chromiumos/platform2/+/master/power_manager/powerd/system/ambient_light_sensor.cc. The uinputdevice.h file that you quoted should probably do something similar. I agree with Mike that we should continue using angle brackets for "system headers" like libchrome/libbase.
,
Mar 14 2018
In https://chromium.googlesource.com/chromiumos/platform2/+/master/CPPLINT.cfg the order is only restricted to alphabetical order. I agree that a lot of projects follow the correct ordering, but if we had the proper pre-hooks, then the mistakes be could detected before submitting. I'm not insisting, on this change. I just thought if something like this would help to make things better, why not!? But you guys think it will cause problems, then probably we shouldn't do it :)
,
Mar 14 2018
Maybe I misunderstood the proposal. I'm completely supportive of improving our pre-upload hooks to prevent weird things like what's in uinputdevice.h. I don't see any reason why all of the files there are mixed together instead of being sorted within separate angle-bracketed libc, STL, and gtest/base/etc. sections like what we do elsewhere in platform2.
,
Mar 14 2018
The proposal was that let's convert angle brackets to double quotes for 'other libraries include files'. Only after we do that, we can move the prehooks to build/include_order from build/include_alpha. Because cpplint always assume that angle brackets are only c or c++ include files. Everything else should be double quote. But if we cannot change 'other libraries include files' to double quotes, then we cannot turn on build/include_order. But I think at the minimum we can fix some of these order problems in some of the projects to conform with proper c++ style. |
||
►
Sign in to add a comment |
||
Comment 1 by vapier@chromium.org
, Mar 14 2018Cc: derat@chromium.org nya@chromium.org benchan@chromium.org
Components: OS>Systems