Opened on 02/14/2018 at 02:51:19 PM
Last modified on 11/06/2018 at 11:12:51 AM
#6390 new change
Make using of quotes and angle brackets consistent for #include directives
Reported by: | sergz | Assignee: | |
---|---|---|---|
Priority: | P4 | Milestone: | |
Module: | Libadblockplus | Keywords: | goodfirstbug |
Cc: | hfiguiere | Blocked By: | |
Blocking: | Platform: | Unknown / Cross platform | |
Ready: | yes | Confidential: | no |
Tester: | Unknown | Verified working: | no |
Review URL(s): |
Description
Libadblockplus follows the following convention
- system and any library headers are included as <header> in order to avoid additional searching steps (and, as a side effect, possible collisions)
- public headers are included as <header> if they are not relative (see below) and
- private headers are included as "header" because it is the only way they can be included.
Here I would like to say that search paths for third-party headers should be actually specified by -isystem if it's possible because there are no real reasons to see warnings arising from those headers.
What concerns the public headers included by other public headers, they should be included via double quotes not angel brackets, e.g. in AdblockPlus.h only one header (#include "AdblockPlus/Notification.h") is included as it's supposed to be, the rest are included incorrectly. The same in FilterEngine.h, instead of #include <AdblockPlus/JsEngine.h> there should be #include "JsEngine.h". Basically they force a user of the library to add the path to a directory containing "AdblockPlus" headers directory to the list of search paths, but IMO a library should not force such things. Another example is JsContext.h where the header AdblockPlus/JsEngine.h is in angle brackets so we already know that it's not available via a relative path and we know that cpp files compiled in libadblockplus are compiled with the corresponding -I parameters. Sometimes it also serves as an addition proof that all public headers are provided, e.g. using "AdblockPlus/JsEngine.h" could lead to the situation when there is AdblockPlus/JsEngine.h but in src directory (thus missing in include public directory), which is not available for a user of the library.
BTW currently there can be the following problem with the public headers. If a user somehow installs libadblockplus headers let's say into /usr/include and wants to try another version of libadblockplus headers, then, when the users includes a public header from another version e.g. by #include "path-to-some-libadblockplus/include/AdblockPlus/FilterEngine.h", the headers from /usr/include/AdblockPlus will be used instead of the headers next to path-to-some-libadblockplus/include/AdblockPlus/FilterEngine.h.
What to change
Go through the #include directives and fix them to be consistent with the convention described above.
Attachments (0)
Change History (2)
comment:1 Changed on 11/01/2018 at 03:38:07 PM by flickz
comment:2 Changed on 11/06/2018 at 11:12:51 AM by sergz
- Cc hfiguiere added
Thanks a lot for the efforts, you can send a merge request on https://gitlab.com/eyeo/adblockplus/libadblockplus/.
Took this to be my first contribution, now stuck with submitting the patch, any help on how to submit the patch for this? Or Will sending a Github PR be better?