Noissue - Use angle brackets include for gtest

Noissue - Use angle brackets include for gtest

[sergei, 2018-02-13 16:03:11]

What about "compiled/String.h" and others?

[hub, 2018-02-13 16:14:29]

On 2018/02/13 16:03:11, sergei wrote:
> What about "compiled/String.h" and others?

They don't have gtest. This is only for gtest/gtest.h since it was a comment in
the other review. For consistency sake (and also libadblockplus does it that
way, so we are consitent was that one as well)

[hub, 2018-02-13 16:16:08]

On 2018/02/13 16:14:29, hub wrote:
> On 2018/02/13 16:03:11, sergei wrote:
> > What about "compiled/String.h" and others?
> 
> They don't have gtest. This is only for gtest/gtest.h since it was a comment
in
> the other review. For consistency sake (and also libadblockplus does it that
> way, so we are consitent was that one as well)

I think I misunderstood. No, the headers for what we are testing should be "".
they are not "system". This is also consistent with libadblockplus.

[sergei, 2018-02-14 15:00:12]

On 2018/02/13 16:16:08, hub wrote:
> On 2018/02/13 16:14:29, hub wrote:
> > On 2018/02/13 16:03:11, sergei wrote:
> > > What about "compiled/String.h" and others?
> > 
> > They don't have gtest. This is only for gtest/gtest.h since it was a comment
> in
> > the other review. For consistency sake (and also libadblockplus does it that
> > way, so we are consitent was that one as well)
> 
> I think I misunderstood. No, the headers for what we are testing should be "".
> they are not "system". This is also consistent with libadblockplus.

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.

[hub, 2018-02-14 15:53:54]

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.

[sergei, 2018-02-27 15:41:10]

On 2018/02/14 15:53:54, hub wrote:
> ... Since adblockpluscore has no public headers, we should specify all its
headers
in ""...
I think we can use that reasoning so far but in general I'm not convinced :)
LGTM

[hub, 2018-02-27 16:31:34]

On 2018/02/27 15:41:10, sergei wrote:
> On 2018/02/14 15:53:54, hub wrote:
> > ... Since adblockpluscore has no public headers, we should specify all its
> headers
> in ""...
> I think we can use that reasoning so far but in general I'm not convinced :)
> LGTM

For the record, my goal in this patch was just to bring consistency with the
other files.