Skip to content
This repository has been archived by the owner on Apr 12, 2024. It is now read-only.

Don't use anonymous namespace in header file. #287

Merged
merged 2 commits into from Apr 3, 2017

Conversation

hokein
Copy link
Contributor

@hokein hokein commented Mar 31, 2017

Anonymous namespace should be forbidden in header files even for the forward declarations:

  • As declarations defined in anonymous namespace are internal linkage, each translation unit which includes this header will get unique copy, which wastes space.
  • It is easy to violate C++ ODR rule.

Consider the following header foo.h:

namespace { class Foo; }
class Bar {
  public:
    Foo* getFoo();
    Foo* foo;
};

If the 'foo.h' is included in multiple .cc files, the compiler will put Foo into a different anonymous namespace in each .cc, which means there are different definitions of Foo in the program (a
violation of the ODR).

\cc @deepak1556

@hokein hokein force-pushed the no-anonymous-ns-in-header branch 2 times, most recently from d97f21c to 59df68d Compare April 1, 2017 06:28
Anonymous namespace should be forbidden in header files even for the
forward declarations:

* As declarations defined in anonymous namespace are internal linkage, each
translation unit which includes this header will get unique copy, which
wastes space.
* It is easy to violate C++ ODR rule.

Consider the following "foo.h":

```cpp
namespace { class Foo; }
class Bar {
  public:
    Foo* getFoo();
    Foo* foo;
}
```

If the 'foo.h' is included in multiple `.cc` files, the compiler will
put `Foo` into a different anonymous namespace in each `.cc`, which
means there are different definitions of `Foo` in the program (a
violation of the ODR).
@hokein hokein force-pushed the no-anonymous-ns-in-header branch from 59df68d to 22341a7 Compare April 1, 2017 06:57
@@ -203,7 +203,8 @@ void InspectableWebContentsViewViews::SetContentsResizingStrategy(

void InspectableWebContentsViewViews::SetTitle(const base::string16& title) {
if (devtools_window_) {
GetDevToolsWindowDelegate()->SetWindowTitle(title);
static_cast<DevToolsWindowDelegate*>(devtools_window_delegate_)
->SetWindowTitle(title);
Copy link
Contributor

Choose a reason for hiding this comment

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

What do you think about moving the ownership of title from DevToolsWindowDelegate to InspectableWebContentsViewViews to avoid the cast ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good.

Copy link
Contributor

@deepak1556 deepak1556 left a comment

Choose a reason for hiding this comment

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

LGTM

@hokein hokein force-pushed the no-anonymous-ns-in-header branch from 8c4d133 to 3a61f08 Compare April 1, 2017 13:11
@kevinsawicki kevinsawicki merged commit baccc07 into master Apr 3, 2017
@kevinsawicki
Copy link
Contributor

Thanks @hokein 👍

@kevinsawicki kevinsawicki deleted the no-anonymous-ns-in-header branch April 3, 2017 15:27
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants