Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(8)

Issue 29696595: Noissue - Use angle brackets include for gtest

Can't Edit
Can't Publish+Mail
Start Review
Created:
5 days, 6 hours ago by hub
Modified:
4 days, 6 hours ago
Reviewers:
sergei
Base URL:
https://hg.adblockplus.org/adblockpluscore/
Visibility:
Public.

Description

Noissue - Use angle brackets include for gtest

Patch Set 1 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+4 lines, -4 lines) Patch
M test/compiled/RegExp.cpp View 1 chunk +1 line, -1 line 0 comments Download
M test/compiled/String.cpp View 1 chunk +1 line, -1 line 0 comments Download
M test/compiled/StringMap.cpp View 1 chunk +1 line, -1 line 0 comments Download
M test/compiled/abptest.cpp View 1 chunk +1 line, -1 line 0 comments Download

Messages

Total messages: 6
hub
5 days, 6 hours ago (2018-02-13 15:51:08 UTC) #1
sergei
What about "compiled/String.h" and others?
5 days, 5 hours ago (2018-02-13 16:03:11 UTC) #2
hub
On 2018/02/13 16:03:11, sergei wrote: > What about "compiled/String.h" and others? They don't have gtest. ...
5 days, 5 hours ago (2018-02-13 16:14:29 UTC) #3
hub
On 2018/02/13 16:14:29, hub wrote: > On 2018/02/13 16:03:11, sergei wrote: > > What about ...
5 days, 5 hours ago (2018-02-13 16:16:08 UTC) #4
sergei
On 2018/02/13 16:16:08, hub wrote: > On 2018/02/13 16:14:29, hub wrote: > > On 2018/02/13 ...
4 days, 6 hours ago (2018-02-14 15:00:12 UTC) #5
hub
4 days, 6 hours ago (2018-02-14 15:53:54 UTC) #6
On 2018/02/14 15:00:12, sergei wrote:

> I think we need to discuss it, so below is my opinion. I would say it should
be
> done in another place, perhaps in hub, if it goes on I will move it there.
> 
> I'm not sure that <> should be considered for system-only headers. In general
> the searching is compiler dependent and the common thing is that <files> are
> searched among the list of search paths passed to the compiler (e.g. -I or /I)
> and then among predefined search paths (if the latter is not disabled), on the
> other hand, "files" are firstly searched in the relative to current directory
> (Visual C++: then in directories of already opened headers (what I find pretty
> confusing and dangerous, though interesting)) and then the searching continues
> as for <file> form. System-only headers should be defined IMO differently,
> namely the search path for them should be rather specified by -isystem
> (unfortunately it's possible only for gcc-like compilers) parameter.
> 
> I have filed a ticket specifying the convention used in libadblockplus and
what
> should be done there https://issues.adblockplus.org/ticket/6390.
> 
> So, what concerns adblockpluscore. For present there are no private and public
> headers. The adblockpluscore headers, which are included in the tests, found
> using the search paths (in accordance with the second parameter of `"-I."
> "-I..\\.."` added as compiler args), so they should be included in angle
> brackets, so it improves the understandability of the project (because it's
> immediately clear what headers can be included by a user via search paths and
> what headers are "private") and ensures that all headers are provided.

According to the standard the difference between #include "" and #include <> is
that with "" it search for the header first relative to the directory where the
file whose #include directive being processed is located. If it doesn't find it,
it revert to the same search path as <>. On gcc there is -iquote and -isystem
(clang support them too) to override the behaviour out of the spec, but we
shouldn't rely on that.

Therefor. "" is for local include and <> for global includes, including
third-party libraries. Behaviour and semantics are related.

Since adblockpluscore has no public headers, we should specify all its headers
in "". Even in the tests. Case in point: "String.h" is getting confused with the
string.h in /usr/include on macOS since the filesystem is case insensitive.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 87257f5