Non-relative includes, restructure, add missing includes #805

NeroBurner · 2021-11-01T20:50:22Z

This is a two-part PR, first use relative includes, and secondly restructure all includes.

The non-relative includes are needed for me in #743 to inject my header-stubs for the simulator and is a more robust coding style for header files

First commit message:

Don't use relative imports like ../foo.h as those depend on the
relative position of both files. Rather than that use imports relative
to the src directory, which explicitly is part of the include
directories.

Second commit message:

Update the include structure:

use #include "foo.h" for headers from the InfiniTime project (except
bundled libraries)
use #include <foo.h> for everything else (external libraries or standard
library)

Change the include order from the most project specific down to the most
generic include. This helps to enforce, that our headers include what
they use. For example if foo.h uses std::array but forgets the
include. This won't be cought if main.cpp includes <array> before
foo.h.

For example DeviceInformationService.h did miss <stdint> include for
uint16_t and BootloaderVersion.h missed <stddef> for size_t.
As last example InfiniPaint.cpp missed include <algorithm> for std::fill.

The following include order is used:

project headers
nimble library
littlefs library
date library
lvgl library
FreeRTOS library
nrf5 sdk
standard libraries

Avamander · 2021-11-01T20:53:12Z

src/components/alarm/AlarmController.cpp

+#include <app_timer.h>
+// FreeRTOS library
+#include <task.h>
+// standard libraries


I don't think the comments are necessary.

They add a bit of structure for future contributions to keep the include order. And it would have been helpful for my first exploration of the source code. For example I had no idea, that <task.h> is part of FreeRTOS. With the comment this is explicitly listet

Sounds like a dev documentation addition instead of each file.

Avamander · 2021-11-01T21:13:11Z

Includes were sorted once a while ago, I don't see a reason to redo that part with the potential downside that InfiniTime's code conflicts with any of the libraries'.

Original sort order

Sort includes:
* source files include their header files first
* library includes next
* own code includes last
* alphabetical

NeroBurner · 2021-11-01T21:20:25Z

One advantage of the new ordering is, that I was able to find some missing includes, that were hidden by the previous ordering.

I don't understand the comment about conflicting library code, all the targets still compile. Would you please be so kind to explain this point to me so I can understand as well?

Avamander · 2021-11-01T21:27:56Z

I don't understand the comment about conflicting library code, all the targets still compile. Would you please be so kind to explain this point to me so I can understand as well?

Yes, at the moment. There'd be a potential for conflicts or nasty bugs when we define something and it ends up being used in a library, because ours comes first instead of being last.

NeroBurner · 2021-11-02T13:01:59Z

I think we won't (and shouldn't) define so short and generic macros, that they get picked up in libraries. Those changes would be bad code for me (personally).

Even if that happens we could move those specific library includes up and still get the "include-what-you-use"-checkings

Another way to argue is, that library defines could bleed into our headers and subtly break stuff. I think the argument works in both directions

sorry for being pedantic about this. I just think it is "more right" and more resilient with the new ordering. In the end it's your project and your guidelines and I'll adhere to them (after a proper discussion 😁 )

Avamander · 2021-11-02T13:06:39Z

Another way to argue is, that library defines could bleed into our headers and subtly break stuff. I think the argument works in both directions

Yes, but from the direction of libraries to us it's less error prone.

Avamander · 2021-11-02T17:06:21Z

I think we won't (and shouldn't) define so short and generic macros, that they get picked up in libraries. Those changes would be bad code for me (personally).

Yes absolutely.

Basically the thing is that I see a very minor upside to not changing the order (fewer changes + our code affects only our own), but I think e.g. @geekbozu or @Riksu9000 should break the tie here.

Riksu9000 · 2021-11-03T11:40:53Z

I would agree with @Avamander here. I don't see enough reason to change it.

src/components/alarm/AlarmController.cpp

NeroBurner · 2021-11-03T22:26:46Z

reverted ordering commit, readd the found missing standard includes

Riksu9000 · 2021-11-04T10:28:17Z

Since we're already restructuring, maybe the different sections should be separated by an empty line, which would make the sorting order more obvious and then clang-tidy wouldn't suggest sorting them in a different way.

Riksu9000 · 2021-11-05T09:09:44Z

If every include must be non relative, it should be mentioned here https://github.com/InfiniTimeOrg/InfiniTime/blob/develop/doc/contribute.md#coding-style

NeroBurner · 2021-11-05T22:58:10Z

moving up the file header include to the top, one example from WatchFaceAnalog.cpp:

#include "displayapp/screens/WatchFaceAnalog.h"
#include <lvgl/lvgl.h>
#include "displayapp/screens/BatteryIcon.h"
#include "displayapp/screens/BleIcon.h"
#include "displayapp/screens/Symbols.h"
#include "displayapp/screens/NotificationIcon.h"

Is this OK? Or should there be a space before and after the lvgl/lvgl.h include?

NeroBurner · 2021-11-05T23:03:22Z

If every include must be non relative, it should be mentioned here https://github.com/InfiniTimeOrg/InfiniTime/blob/develop/doc/contribute.md#coding-style

@Riksu9000 is this Ok? 0d745a0

Avamander

Seems fine now. Someone should take a look as well.

Avamander · 2021-11-07T11:33:25Z

Oh no, a conflict, if you fix that and we can merge, we can avoid breaking this PR with each and every other PR.

NeroBurner · 2021-11-07T12:48:22Z

rebased and conflict resolved

NeroBurner · 2021-11-15T19:31:31Z

rebased on current develop (after v1.7.0 release)

Don't use relative imports like `../foo.h` as those depend on the relative position of both files. Rather than that use imports relative to the `src` directory, which explicitly is part of the include directories.

NeroBurner · 2021-11-15T21:04:43Z

rebased on develop after v1.7.1 release

NeroBurner · 2021-11-22T22:07:52Z

@Avamander Is something still missing, did I forget something? Or is it just limited free time for this awesome hobby? :)

Avamander · 2021-11-22T22:12:50Z

Everything seems to be in order, I think it's in the queue waiting for a merge.

JF002 · 2021-11-28T12:56:24Z

@NeroBurner

Is something still missing, did I forget something? Or is it just limited free time for this awesome hobby? :)

I'm sorry for the time it took for me to have a look at your PR! I am currently the only one that is allowed to merge PRs to develop (that might change in the future) and I couldn't allocate much time to InfiniTime recently!

However, I think you did a great job cleaning the includes of the project! Thank you for that!
I've just fixed the last conflicts that were introduce by PRs I've just merged and I merged this one!

NeroBurner · 2021-11-28T18:46:34Z

@JF002 Thanks 🎉 Happy it's merged

InfiniTime is a hobby-project. If there is no personal time left to work on it, then it is ok as well ;) It's true, that waiting for feedback for weeks was bad for motivation, but now I'm happy my contribution got merged.

I'm VERY grateful to you, that you created InfiniTime. Better take some personal time, than burning out on the project

In short: thank you @JF002 :)

JF002 · 2021-11-29T20:29:22Z

@NeroBurner Thank you so much for your kind words! Do not forget I'm not alone in this adventure! There are so many contributors and enthousiasts and users that supports the project! Thank to all of them!
I understand that waiting several weeks for a feedback is not ideal, and that's exactly for that reason that I'm creating teams of contributors that help me to triage the issues, PRs, review the code, help with specific topics,...
Thanks again, and have fun contributing again to the project :-)

Avamander reviewed Nov 1, 2021

View reviewed changes

Avamander changed the title ~~non-relative includes, restructure, add missing includes~~ Non-relative includes, restructure, add missing includes Nov 1, 2021

Riksu9000 reviewed Nov 3, 2021

View reviewed changes

src/components/alarm/AlarmController.cpp Show resolved Hide resolved

Avamander approved these changes Nov 7, 2021

View reviewed changes

NeroBurner force-pushed the restructure_includes branch from 0d745a0 to e42fbce Compare November 7, 2021 12:47

Avamander requested a review from Riksu9000 November 7, 2021 12:50

Riksu9000 approved these changes Nov 7, 2021

View reviewed changes

NeroBurner force-pushed the restructure_includes branch from e42fbce to 41cb6eb Compare November 15, 2021 19:30

NeroBurner added 5 commits November 15, 2021 22:02

Update includes to to be relative to src directory

ac7b2da

Don't use relative imports like `../foo.h` as those depend on the relative position of both files. Rather than that use imports relative to the `src` directory, which explicitly is part of the include directories.

Add missing standard includes

f291870

remove unused libs/ prefix from lvgl includes as not needed

1b937a7

Move up file header include to top

241d364

docs: add non-relative includes to coding standard

3a41bff

NeroBurner force-pushed the restructure_includes branch from 41cb6eb to 3a41bff Compare November 15, 2021 21:04

NeroBurner mentioned this pull request Nov 22, 2021

Chimes option #813

Merged

JF002 merged commit 298f0f4 into InfiniTimeOrg:develop Nov 28, 2021

JF002 added this to the 1.8.0 milestone Nov 28, 2021

NeroBurner deleted the restructure_includes branch November 28, 2021 18:46

Uh oh!

Non-relative includes, restructure, add missing includes #805

Non-relative includes, restructure, add missing includes #805

Uh oh!

Conversation

NeroBurner commented Nov 1, 2021

Uh oh!

Avamander Nov 1, 2021

Choose a reason for hiding this comment

Uh oh!

NeroBurner Nov 1, 2021

Choose a reason for hiding this comment

Uh oh!

Avamander Nov 1, 2021

Choose a reason for hiding this comment

Uh oh!

Avamander commented Nov 1, 2021

Uh oh!

NeroBurner commented Nov 1, 2021

Uh oh!

Avamander commented Nov 1, 2021

Uh oh!

NeroBurner commented Nov 2, 2021

Uh oh!

Avamander commented Nov 2, 2021

Uh oh!

Avamander commented Nov 2, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Riksu9000 commented Nov 3, 2021

Uh oh!

Uh oh!

NeroBurner commented Nov 3, 2021

Uh oh!

Riksu9000 commented Nov 4, 2021

Uh oh!

Riksu9000 commented Nov 5, 2021

Uh oh!

NeroBurner commented Nov 5, 2021

Uh oh!

NeroBurner commented Nov 5, 2021

Uh oh!

Avamander left a comment

Choose a reason for hiding this comment

Uh oh!

Avamander commented Nov 7, 2021

Uh oh!

NeroBurner commented Nov 7, 2021

Uh oh!

NeroBurner commented Nov 15, 2021

Uh oh!

NeroBurner commented Nov 15, 2021

Uh oh!

NeroBurner commented Nov 22, 2021

Uh oh!

Avamander commented Nov 22, 2021

Uh oh!

JF002 commented Nov 28, 2021

Uh oh!

NeroBurner commented Nov 28, 2021

Uh oh!

JF002 commented Nov 29, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Avamander commented Nov 2, 2021 •

edited

Loading