Skip to content

Conversation

@mathieucarbou
Copy link
Member

Wit hsome filters that are not impacting a lot the current code

@vortigont
Copy link

it removed all override and explicit keywords, even in comments :)))

@mathieucarbou
Copy link
Member Author

it removed all override and explicit keywords, even in comments :)))

for explicit, this is my mistakes.

Also, I disabled the explicit check: runtime/explicit : I do not know how much it is relevant to put explicit on ctors who need it ?

About override, this is not necessary when final is there. If final is there it implicitly means override.

cpplint justs check: I did the cleaup.

@mathieucarbou
Copy link
Member Author

FYI, please review / approve but please do not merge yet: I would prefer to do the release first and merge after. Thanks!

@vortigont
Copy link

OK, if it's only with finals.
For explicit it could be useful sometimes for constructors having non-default args to ensure it accepts only specified type.

Copy link

@vortigont vortigont left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks Ok'ish )

#include <memory>
#include <vector>

#include "./literals.h"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why relative? chance to have another header with the same name elsewhere?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no - that's just a cpplint request to have path indications when using "..." instead of <...> in order to have the includes formatted the same way everywhere. I think this one is ok, but we can disable this check if we want.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's not the same though. Well it will point to the same file, but not in the same way. "literals.h" is file in one of the include paths, while "./literals.h" means file in the same folder as the file which includes it that way

Copy link
Member Author

@mathieucarbou mathieucarbou Nov 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes I know the difference...

I think cpplint uses this programming convention:

  • "./path/to/include.h"
  • <from/compile/path/include/h>

They "ask" devs to follow this convention (if we keep the flag).

I am personally not against this convention since it adds more clarity when reading the code.
Currently, this is a mess in our code base for the includes. There is no strict conventions.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it will work either way in this case (relative or not), but just last few days we've been dealing with some relative paths in ESP-IDF too and they've proven to be messy. Anyway, I just wanted to know why did you opt for relative path. Otherwise, my understanding is that "header.h" is library/project local include, while <header.h> is something that comes from elsewhere (another library)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Otherwise, my understanding is that "header.h" is library/project local include, while <header.h> is something that comes from elsewhere (another library)

Yes the rational comes from that.
And that's also the convention cpplint is enforcing with this check.
So if OK we can go with that for the project from now on, and maybe on day take the opportunity to cleanup the existing includes.

Copy link
Member

@me-no-dev me-no-dev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants