Moving file_util to base

[Matt Giuca]

I have noticed that over the past few weeks that many functions from the file_util namespace are being moved into the base namespace (a handful at a time). Some of the recent CLs to do so are:

https://codereview.chromium.org/19052005/

https://codereview.chromium.org/18286004/

I can't find any announcement on chromium-dev about this and none of the CLs link to a particular bug. I was wondering if we could get an explanation on why this is happening.

IMHO, it seems as though this defeats the purpose of namespaces, and makes function names less descriptive (e.g., file_util::ContentsEqual implies that it is comparing files whereas base::ContentsEqual doesn't really mean much). This seems to have already caused some problems that needed further refactoring: after file_util::Delete was renamed to the (meaningless) base::Delete, it had to be renamed again to base::DeleteFile. It seems like instead of renaming everything to have the word "File" in the name, it would have been much easier to just make no changes in the first place.

(And there is a somewhat-hidden cost to these giant refactors -- each of these causes merge conflicts with potentially hundreds of unsubmitted CLs that are still in development.)

So just wondering what the rationale was. Thoughts?

Matt

[Peter Kasting]

On Thu, Jul 11, 2013 at 5:41 PM, Matt Giuca <mgi...@chromium.org> wrote:

I can't find any announcement on chromium-dev about this and none of the CLs link to a particular bug. I was wondering if we could get an explanation on why this is happening.

My understanding is that we're trying to move towards things in base/ being in base::, and ultimately things in top_level_dir/ being in top_level_dir:: in general.  This is in line with the Google-internal version of the Google C++ style guide (unfortunately, the publicly-available version omits the directives on choosing namespace names, other than to say "choose the name based on the project, and possibly its path"), and is also in line with e.g. the cc:: namespace for the compositor.

Classically, our usage of namespaces has been ad-hoc and inconsistent, and have a file_util:: namespace for stuff just defined in a random header in base/ is certainly a case of this.

IMHO, it seems as though this defeats the purpose of namespaces, and makes function names less descriptive (e.g., file_util::ContentsEqual implies that it is comparing files whereas base::ContentsEqual doesn't really mean much).

I agree that base::ContentsEqual() is insufficiently meaningful. I disagree that file_util::ContentsEqual() is sufficiently meaningful.  Even for qualified usages, this could very well refer to the contents of a file path or something.  Worse would be any usage inside the namespace, or in code that uses using directives to unqualify the usages, because then those become ContentsEqual() alone, which is just as bad as what you're complaining about.

This function ought to be named FileContentsEqual() regardless of its namespace.

This seems to have already caused some problems that needed further refactoring: after file_util::Delete was renamed to the (meaningless) base::Delete, it had to be renamed again to base::DeleteFile. It seems like instead of renaming everything to have the word "File" in the name, it would have been much easier to just make no changes in the first place.

As you can probably guess from the example above, I view DeleteFile() as a much better name for this function regardless of its namespace, so this doesn't seem like useless churn to me.

(I suspect in this case the double-rename might have been due to a Koenig lookup issue Brett was telling me about yesterday.)

PK

[Jonathan Dixon]

On 11 July 2013 17:50, Peter Kasting <pkas...@chromium.org> wrote:

On Thu, Jul 11, 2013 at 5:41 PM, Matt Giuca <mgi...@chromium.org> wrote:

I can't find any announcement on chromium-dev about this and none of the CLs link to a particular bug. I was wondering if we could get an explanation on why this is happening.

My understanding is that we're trying to move towards things in base/ being in base::, and ultimately things in top_level_dir/ being in top_level_dir:: in general.  This is in line with the Google-internal version of the Google C++ style guide (unfortunately, the publicly-available version omits the directives on choosing namespace names, other than to say "choose the name based on the project, and possibly its path"), and is also in line with e.g. the cc:: namespace for the compositor.

The style guide also has this somewhat vague statement: "Rather than creating classes only to group static member functions which do not share static data, use namespaces instead."

Given a cursory reading of that clause out of context, I could imagine "class FileUtil" became "namespace file_util" in some prior refactor. I've certainly seen similar reasoning applied elsewhere.

 

Classically, our usage of namespaces has been ad-hoc and inconsistent, and have a file_util:: namespace for stuff just defined in a random header in base/ is certainly a case of this.

IMHO, it seems as though this defeats the purpose of namespaces, and makes function names less descriptive (e.g., file_util::ContentsEqual implies that it is comparing files whereas base::ContentsEqual doesn't really mean much).

I agree that base::ContentsEqual() is insufficiently meaningful. I disagree that file_util::ContentsEqual() is sufficiently meaningful.  Even for qualified usages, this could very well refer to the contents of a file path or something.  Worse would be any usage inside the namespace, or in code that uses using directives to unqualify the usages, because then those become ContentsEqual() alone, which is just as bad as what you're complaining about.

This function ought to be named FileContentsEqual() regardless of its namespace.

This seems to have already caused some problems that needed further refactoring: after file_util::Delete was renamed to the (meaningless) base::Delete, it had to be renamed again to base::DeleteFile. It seems like instead of renaming everything to have the word "File" in the name, it would have been much easier to just make no changes in the first place.

As you can probably guess from the example above, I view DeleteFile() as a much better name for this function regardless of its namespace, so this doesn't seem like useless churn to me.

(I suspect in this case the double-rename might have been due to a Koenig lookup issue Brett was telling me about yesterday.)

PK

--
--
Chromium Developers mailing list: chromi...@chromium.org
View archives, change email options, or unsubscribe:
http://groups.google.com/a/chromium.org/group/chromium-dev
 
 
 

[Brett Wilson]

On Thu, Jul 11, 2013 at 5:50 PM, Peter Kasting <pkas...@chromium.org> wrote:
> On Thu, Jul 11, 2013 at 5:41 PM, Matt Giuca <mgi...@chromium.org> wrote:
>>
>> I can't find any announcement on chromium-dev about this and none of the
>> CLs link to a particular bug. I was wondering if we could get an explanation
>> on why this is happening.
>
>
> My understanding is that we're trying to move towards things in base/ being
> in base::, and ultimately things in top_level_dir/ being in top_level_dir::
> in general. This is in line with the Google-internal version of the Google
> C++ style guide (unfortunately, the publicly-available version omits the
> directives on choosing namespace names, other than to say "choose the name
> based on the project, and possibly its path"), and is also in line with e.g.
> the cc:: namespace for the compositor.
>
> Classically, our usage of namespaces has been ad-hoc and inconsistent, and
> have a file_util:: namespace for stuff just defined in a random header in
> base/ is certainly a case of this.

Yes, I'm trying to slowly make the base namespace usage consistent,
just like we're doing for content.

I agree some names are vague. I just did a patch to rename Delete ->
DeleteFile, and ContentsEqual is another case like this. Initially I
was avoiding doing this type of renaming at the same time due to a bad
experience at the beginning, but assuming the DeleteFile patch goes
OK, I will probably be more aggressive about doing renaming at the
same time as moving namespaces.

I wouldn't expect chromium-dev announcements every time a file changes
namespaces. And the base cleanup (fixing namespaces, splitting apart
huge *_util files, categorizing the directory hierarchy) has been
happening in the background for >2 years and I expect will (slowly)
continue for quite some time.

My recommended namespace usage is that it should match the toplevel
module (base, net, content, etc.) and that you can use the two common
Google sub-namespace ones ("subtle" and "internal") as necessary, but
try to avoid random per-file namespaces. However, if a different
namespace is the right answer for your problem, go ahead. We're
probably not doing a "chrome" namespace, both for practicality and
because chrome should be a leaf node so a namespace isn't so useful.

Brett

[Peter Kasting]

On Fri, Jul 12, 2013 at 10:13 AM, Brett Wilson <bre...@chromium.org> wrote:

We're

probably not doing a "chrome" namespace, both for practicality and
because chrome should be a leaf node so a namespace isn't so useful.

We do actually have a chrome namespace today, with a completely random set of things in it.  It makes no sense to me; is it safe to rip out?

PK 

[Chris Hopman]

I would prefer adding the chrome:: namespace everywhere it is missing. It seems strange to have such an inconsistency when there is no benefit (that I can think of) to not having the namespace. 

--

[Peter Kasting]

On Fri, Jul 12, 2013 at 1:17 PM, Chris Hopman <cjho...@chromium.org> wrote:

I would prefer adding the chrome:: namespace everywhere it is missing. It seems strange to have such an inconsistency when there is no benefit (that I can think of) to not having the namespace.

The benefit to not having the namespace is avoiding things like "namespace chrome {" and "using chrome::" and "chrome::" everywhere.  And considering the amount of code inside chrome/, adding a namespace to the 99.999% of code that doesn't already use it is pretty much infeasible.

PK 

[Brett Wilson]

I think in many cases it is possible to rip out, although I think
there are a few cases where we may need to be careful to avoid some
confusion.

Brett

[Brett Wilson]

On Fri, Jul 12, 2013 at 1:17 PM, Chris Hopman <cjho...@chromium.org> wrote:

> I would prefer adding the chrome:: namespace everywhere it is missing. It
> seems strange to have such an inconsistency when there is no benefit (that I
> can think of) to not having the namespace.

There are ~23K files. This project is not worth doing for so little
marginal value.

Brett

[John Abd-El-Malek]

I think that value is negative, for similar reasons that Peter mentioned. 

Brett