Issue 29613616: Issue 6064 - Put C++ code into a configurable namespace

Issue 29613616: Issue 6064 - Put C++ code into a configurable namespace (Closed)

Created:
Nov. 21, 2017, 1:53 p.m. by sergei

Modified:
Feb. 7, 2018, 2:44 p.m.

Reviewers:
hub

Base URL:
https://github.com/adblockplus/adblockpluscore.git

Visibility:
Public.

Patch Set 2 : Fix debug.h to be aligned with assert2 (define vs function) and get rid of a warning in generator.cpp #

Total comments: 8

Patch Set 3 : address comments #

Patch Set 5 : rebase #

Total comments: 4

Created: Feb. 6, 2018, 9:54 a.m.

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+250 lines, -40 lines)			Patch
M	compiled/ElemHide.h	View	1 2 3 4	3 chunks	+4 lines, -1 line	0 comments	Download
M	compiled/ElemHide.cpp	View	1 2 3 4	1 chunk	+2 lines, -0 lines	0 comments	Download
M	compiled/ElemHideEmulation.h	View	1 2 3 4	2 chunks	+4 lines, -0 lines	0 comments	Download
M	compiled/ElemHideEmulation.cpp	View	1 2 3 4	1 chunk	+2 lines, -0 lines	0 comments	Download
M	compiled/FilterNotifier.h	View		2 chunks	+5 lines, -0 lines	0 comments	Download
M	compiled/IntMap.h	View	1 2 3 4	2 chunks	+5 lines, -0 lines	0 comments	Download
M	compiled/Map.h	View	1 2 3 4	2 chunks	+5 lines, -0 lines	0 comments	Download
M	compiled/String.h	View	1 2 3 4	2 chunks	+4 lines, -0 lines	0 comments	Download
M	compiled/StringMap.h	View	1 2 3 4	3 chunks	+7 lines, -3 lines	1 comment	Download
M	compiled/StringScanner.h	View		2 chunks	+5 lines, -0 lines	0 comments	Download
M	compiled/Utils.h	View	1 2 3 4	1 chunk	+5 lines, -0 lines	0 comments	Download
M	compiled/Utils.cpp	View	1 2 3 4	1 chunk	+3 lines, -0 lines	0 comments	Download
A +	compiled/base.h	View	1 2	1 chunk	+8 lines, -6 lines	3 comments	Download
M	compiled/bindings/generator.h	View		2 chunks	+5 lines, -0 lines	0 comments	Download
M	compiled/bindings/generator.cpp	View	1	3 chunks	+6 lines, -2 lines	0 comments	Download
M	compiled/bindings/main.cpp	View	1 2 3 4	1 chunk	+2 lines, -0 lines	0 comments	Download
M	compiled/bindings/runtime_utils.cpp	View		1 chunk	+2 lines, -0 lines	0 comments	Download
M	compiled/debug.h	View	1	5 chunks	+12 lines, -1 line	0 comments	Download
M	compiled/filter/ActiveFilter.h	View	1 2 3 4	3 chunks	+7 lines, -2 lines	0 comments	Download
M	compiled/filter/ActiveFilter.cpp	View	1 2 3 4	1 chunk	+2 lines, -0 lines	0 comments	Download
M	compiled/filter/BlockingFilter.h	View	1 2 3	2 chunks	+5 lines, -0 lines	0 comments	Download
M	compiled/filter/BlockingFilter.cpp	View	1 2 3	1 chunk	+2 lines, -0 lines	0 comments	Download
M	compiled/filter/CommentFilter.h	View	1 2 3	2 chunks	+5 lines, -0 lines	0 comments	Download
M	compiled/filter/CommentFilter.cpp	View	1 2 3	1 chunk	+2 lines, -0 lines	0 comments	Download
M	compiled/filter/ElemHideBase.h	View	1 2 3	2 chunks	+5 lines, -0 lines	0 comments	Download
M	compiled/filter/ElemHideBase.cpp	View	1 2 3	1 chunk	+2 lines, -0 lines	0 comments	Download
M	compiled/filter/ElemHideEmulationFilter.h	View	1 2 3	2 chunks	+5 lines, -0 lines	0 comments	Download
M	compiled/filter/ElemHideEmulationFilter.cpp	View	1 2 3	1 chunk	+2 lines, -0 lines	0 comments	Download
M	compiled/filter/ElemHideException.h	View	1 2 3	2 chunks	+5 lines, -0 lines	0 comments	Download
M	compiled/filter/ElemHideException.cpp	View	1 2 3	1 chunk	+2 lines, -0 lines	0 comments	Download
M	compiled/filter/ElemHideFilter.h	View	1 2 3	2 chunks	+5 lines, -0 lines	0 comments	Download
M	compiled/filter/ElemHideFilter.cpp	View	1 2 3	1 chunk	+2 lines, -0 lines	0 comments	Download
M	compiled/filter/Filter.h	View	1 2 3	2 chunks	+5 lines, -0 lines	0 comments	Download
M	compiled/filter/Filter.cpp	View	1 2 3	1 chunk	+2 lines, -0 lines	0 comments	Download
M	compiled/filter/InvalidFilter.h	View	1 2 3	2 chunks	+5 lines, -0 lines	0 comments	Download
M	compiled/filter/InvalidFilter.cpp	View	1 2 3	1 chunk	+2 lines, -0 lines	0 comments	Download
M	compiled/filter/RegExpFilter.h	View	1 2 3	2 chunks	+5 lines, -0 lines	0 comments	Download
M	compiled/filter/RegExpFilter.cpp	View	1 2 3	1 chunk	+2 lines, -0 lines	0 comments	Download
M	compiled/filter/WhitelistFilter.h	View	1 2 3	2 chunks	+5 lines, -0 lines	0 comments	Download
M	compiled/filter/WhitelistFilter.cpp	View	1 2 3	1 chunk	+2 lines, -0 lines	0 comments	Download
M	compiled/intrusive_ptr.h	View		2 chunks	+5 lines, -0 lines	0 comments	Download
M	compiled/library.h	View		2 chunks	+14 lines, -8 lines	0 comments	Download
M	compiled/library.cpp	View	1 2 3 4	1 chunk	+1 line, -0 lines	0 comments	Download
M	compiled/storage/FilterStorage.h	View		2 chunks	+5 lines, -0 lines	0 comments	Download
M	compiled/storage/FilterStorage.cpp	View		1 chunk	+2 lines, -0 lines	0 comments	Download
M	compiled/subscription/DownloadableSubscription.h	View	1 2 3	2 chunks	+5 lines, -0 lines	0 comments	Download
M	compiled/subscription/DownloadableSubscription.cpp	View	1 2 3	1 chunk	+2 lines, -0 lines	0 comments	Download
M	compiled/subscription/Subscription.h	View	1 2 3	4 chunks	+7 lines, -2 lines	0 comments	Download
M	compiled/subscription/Subscription.cpp	View		1 chunk	+2 lines, -0 lines	0 comments	Download
M	compiled/subscription/UserDefinedSubscription.h	View	1 2 3	2 chunks	+5 lines, -0 lines	0 comments	Download
M	compiled/subscription/UserDefinedSubscription.cpp	View	1 2 3	1 chunk	+2 lines, -0 lines	0 comments	Download
M	compiled/traceInit.cpp	View		1 chunk	+15 lines, -10 lines	0 comments	Download
A +	compiled/user-config.h	View	1 2	1 chunk	+14 lines, -5 lines	0 comments	Download
M	test/compiled/RegExp.cpp	View	1 2 3 4	1 chunk	+2 lines, -0 lines	0 comments	Download
M	test/compiled/String.cpp	View	1 2 3 4	1 chunk	+2 lines, -0 lines	0 comments	Download
M	test/compiled/StringMap.cpp	View	1 2 3 4	1 chunk	+2 lines, -0 lines	0 comments	Download

Messages

Total messages: 9

Expand All Messages | Collapse All Messages

hub

https://codereview.adblockplus.org/29613616/diff/29613714/compiled/base.h File compiled/base.h (right): https://codereview.adblockplus.org/29613616/diff/29613714/compiled/base.h#newcode22 compiled/base.h:22: # define ABP_USER_CONFIG "user.h" I would call this "user-config.h". ...

Nov. 21, 2017, 4:48 p.m. (2017-11-21 16:48:40 UTC) #2

sergei

Nov. 22, 2017, 9:39 a.m. (2017-11-22 09:39:28 UTC) #3

https://codereview.adblockplus.org/29613616/diff/29613714/compiled/base.h
File compiled/base.h (right):

https://codereview.adblockplus.org/29613616/diff/29613714/compiled/base.h#new...
compiled/base.h:22: #  define ABP_USER_CONFIG "user.h"
On 2017/11/21 16:48:39, hub wrote:
> I would call this "user-config.h". and the present header config.h instead of
> base.h.
I like to have config.h with a more specific part user-config.h which is also
close to ABP_USER_CONFIG. I actually thought about config/user.h and just
omitted 'config' for the sake of simplicity.
I have renamed user.h to user-config.h.

What concerns base.h, I considered it as not only a place for configuration. For
instance base.h could also include some additional very common headers or have
some project specific defines. So, when we need it in my opinion we should not
go again through all relevant files and change config.h to some base.h.
Therefore I would propose to have
base.h
  include config.h
    include user-config.h
but currently having both config.h and base.h seems redundant, so I would
propose to not complicate it for present, it will be very easy to add config.h
when it's really needed. What do you think about it?

> 
> We could also consider having it generated from the build system.
> 
> But this is just my suggestion.
> 
> Also I would just have the content of "user.h" here if ABP_USER_CONFIG is
> undefined.
The idea is that one can simply copy user-config.h and adjust few parameters, if
it is inlined then one will have to firstly figure out what should be in a
custom user-config.h. So, IMO it should not be inlined.

https://codereview.adblockplus.org/29613616/diff/29613714/compiled/bindings/g...
File compiled/bindings/generator.cpp (right):

https://codereview.adblockplus.org/29613616/diff/29613714/compiled/bindings/g...
compiled/bindings/generator.cpp:29: ABP_NS_BEGIN
On 2017/11/21 16:48:39, hub wrote:
> the anonymous namespace above should be within this namespace.
Here this anonymous namespace can be in ABP_NS namespace but in general I am not
sure that we should make it so strict because in this case one will have to use
`ABP_NS::` prefix sometimes in order to get access to things declared in the
ABP_NS::<unnamed_namespace> what can be rather annoying and even confusing
because there is already ABP_NS_USING. Do you mind to generally not put
anonymous namespaces already residing in our translation unit into ABP_NS if
there is no conflict?

https://codereview.adblockplus.org/29613616/diff/29613714/compiled/filter/Ele...
File compiled/filter/ElemHideEmulationFilter.cpp (right):

https://codereview.adblockplus.org/29613616/diff/29613714/compiled/filter/Ele...
compiled/filter/ElemHideEmulationFilter.cpp:20: ABP_NS_USING
On 2017/11/21 16:48:39, hub wrote:
> This should be ABP_NS_BEGIN
> (and have the matching _END)

Could you please explain the advantage of wrapping the code in ABP_NS_BEGIN
ABP_NS_END here?

https://codereview.adblockplus.org/29613616/diff/29613714/compiled/user.h
File compiled/user.h (right):

https://codereview.adblockplus.org/29613616/diff/29613714/compiled/user.h#new...
compiled/user.h:23: #define ABP_NS_USING
On 2017/11/21 16:48:39, hub wrote:
> I think we should just need to define ABP_NS.
> 
> #define ABP_NS my_namespace
> 
> #if defined(ABP_NS)
> #define ABP_NS_BEGIN namespace ABP_NS {
> #define ABP_NS_END }
> #define ABP_NS_USING using ABP_NS;
> #else
> #define ABP_NS
> #define ABP_NS_BEGIN
> #define ABP_NS_END
> #define ABP_NS_USING
> #endif
> 
> 
> And as I mentionned above, possibly have this in "base.h"

I just tried to keep it small and simple :) but I agree it can be more
user-friendly, added it.

sergei

rebased, can we prioritize it because it seems harder and harder to upload patches with ...

Feb. 6, 2018, 10:03 a.m. (2018-02-06 10:03:01 UTC) #5

sergei

https://codereview.adblockplus.org/29613616/diff/29690786/compiled/StringMap.h File compiled/StringMap.h (right): https://codereview.adblockplus.org/29613616/diff/29690786/compiled/StringMap.h#newcode27 compiled/StringMap.h:27: namespace StringMap_internal here is actually an unrelated change, we ...

Feb. 6, 2018, 10:03 a.m. (2018-02-06 10:03:19 UTC) #6

hub

https://codereview.adblockplus.org/29613616/diff/29690786/compiled/base.h File compiled/base.h (left): https://codereview.adblockplus.org/29613616/diff/29690786/compiled/base.h#oldcode24 compiled/base.h:24: #define BINDINGS_EXPORTED Where did these go?

Feb. 6, 2018, 2:59 p.m. (2018-02-06 14:59:33 UTC) #7

sergei

Feb. 6, 2018, 3:01 p.m. (2018-02-06 15:01:59 UTC) #8

hub

Feb. 6, 2018, 3:05 p.m. (2018-02-06 15:05:02 UTC) #9

LGTM

https://codereview.adblockplus.org/29613616/diff/29690786/compiled/base.h
File compiled/base.h (left):

https://codereview.adblockplus.org/29613616/diff/29690786/compiled/base.h#old...
compiled/base.h:24: #define BINDINGS_EXPORTED
On 2018/02/06 15:01:59, sergei wrote:
> On 2018/02/06 14:59:31, hub wrote:
> > Where did these go?
> 
> base.h is a new file but Rietveld thinks that it's copied from another file,
so
> basically these lines didn't go because they didn't exist.

ok

Expand All Messages | Collapse All Messages