Re: [webkit-dev] namespace indent
Should the namespace-indent rule apply even to nested namespaces? I
understand the reasoning behind the rule — it's a waste of horizontal
space to indent almost everything in the file — but if a secondary
namespace is declared, it would seem weird not to indent its lines
either.
This comes up because I have a patch out for review that includes the
addition of an HTTPHeaders namespace that just contains a bunch of
string constants:
// HTTPHeaderMap.h
namespace WebCore {
class HTTPHeaderMap : public HashMapAtomicString, String,
CaseFoldingHash {
...
...
};
namespace HTTPHeaders {
extern const char* const Accept;
extern const char* const Authorization;
...
extern const char* const UserAgent;
}
}
This would look strange if the contents of HTTPHeaders weren't
indented. Especially since if HTTPHeaders were made a class instead
(which I almost decided to do) the style guide _would_ say to indent it.
namespace HTTPHeaders {
extern const char* const Accept;
extern const char* const Authorization;
...
extern const char* const UserAgent;
}
—Jens
___
webkit-dev mailing list
webkit-dev@lists.webkit.org
http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] namespace indent
On 03.12.2009, at 14:22, Jens Alfke wrote: This comes up because I have a patch out for review that includes the addition of an HTTPHeaders namespace that just contains a bunch of string constants: I do not think constants for HTTP headers are part of HTTPHeaderMap - maybe it would be better to add them to a new file. That would resolve the issue with style automatically, although I agree with the rationale you provided. - WBR, Alexey Proskuryakov ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] namespace indent
On Dec 3, 2009, at 3:40 PM, David Levin wrote: So far it seems at least two people agree with this and no one objected last time. The next appropriate step if you want the issue fixed is to file a bug on the style guide and update it. https://bugs.webkit.org/show_bug.cgi?id=32137 Working on a patch now. Is there a meta-style guide for updates to the style guide? ;) —Jens ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] namespace indent
btw, that you put it this way :) This issue came up before: First, it seems like the original motive was to avoid pointlessly indenting nearly the whole file: https://lists.webkit.org/pipermail/webkit-dev/2009-September/010002.html So, I was wondering if we can clarify the rule to apply only to the outermost namespace declaration. So, I was wondering if we can clarify the rule to apply only to the outermost namespace declaration. [Darin Adler's reply] Yes, I think we can. So far it seems at least two people agree with this and no one objected last time. The next appropriate step if you want the issue fixed is to file a bug on the style guide and update it. dave On Thu, Dec 3, 2009 at 3:33 PM, Alexey Proskuryakov a...@webkit.org wrote: On 03.12.2009, at 14:22, Jens Alfke wrote: This comes up because I have a patch out for review that includes the addition of an HTTPHeaders namespace that just contains a bunch of string constants: I do not think constants for HTTP headers are part of HTTPHeaderMap - maybe it would be better to add them to a new file. That would resolve the issue with style automatically, although I agree with the rationale you provided. - WBR, Alexey Proskuryakov ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
[webkit-dev] namespace indent
A common false positive in the style-queue is as follows: Code inside a namespace should not be indented. [whitespace/indent] [4] That's because the namespace indent rule is fairly new and its hard to fix without touching the whole file. I don't think the style-queue should be spamming bugs with non-actionable information. There seem to be a few choices: 1) Disable this warning because it's not helpful at the moment. 2) Change our code to comply with this style rule (e.g., as we touch files, fix them so that the warning doesn't occur). 3) Change our style guide to match our code. Personally, I'm in favor of (2) because it seems silly to have a rule in our style guide with which we never plan to actually comply. Thoughts? Adam ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] namespace indent
(2). On Dec 1, 2009, at 2:42 PM, Adam Barth wrote: A common false positive in the style-queue is as follows: Code inside a namespace should not be indented. [whitespace/indent] [4] That's because the namespace indent rule is fairly new and its hard to fix without touching the whole file. I don't think the style-queue should be spamming bugs with non-actionable information. There seem to be a few choices: 1) Disable this warning because it's not helpful at the moment. 2) Change our code to comply with this style rule (e.g., as we touch files, fix them so that the warning doesn't occur). 3) Change our style guide to match our code. Personally, I'm in favor of (2) because it seems silly to have a rule in our style guide with which we never plan to actually comply. Thoughts? Adam ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] namespace indent
For (2), are we sure the relevant diff tools can isolate the real changes to those files amid mass indentation? Most if not all can, but I don't know about the automatic diffs that may be part of the webkit build/release/publish process. Not that this is a deal-breaker, but I wanted to bring it up. On Dec 1, 2009, at 12:52 PM, David Hyatt wrote: (2). On Dec 1, 2009, at 2:42 PM, Adam Barth wrote: A common false positive in the style-queue is as follows: Code inside a namespace should not be indented. [whitespace/indent] [4] That's because the namespace indent rule is fairly new and its hard to fix without touching the whole file. I don't think the style-queue should be spamming bugs with non-actionable information. There seem to be a few choices: 1) Disable this warning because it's not helpful at the moment. 2) Change our code to comply with this style rule (e.g., as we touch files, fix them so that the warning doesn't occur). 3) Change our style guide to match our code. Personally, I'm in favor of (2) because it seems silly to have a rule in our style guide with which we never plan to actually comply. Thoughts? Adam ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] namespace indent
The pretty diff on Bugzilla knows how to show you what's changed on a given line (e.g., whitespace). Adam On Tue, Dec 1, 2009 at 1:11 PM, Rudi Sherry rshe...@adobe.com wrote: For (2), are we sure the relevant diff tools can isolate the real changes to those files amid mass indentation? Most if not all can, but I don't know about the automatic diffs that may be part of the webkit build/release/publish process. Not that this is a deal-breaker, but I wanted to bring it up. On Dec 1, 2009, at 12:52 PM, David Hyatt wrote: (2). On Dec 1, 2009, at 2:42 PM, Adam Barth wrote: A common false positive in the style-queue is as follows: Code inside a namespace should not be indented. [whitespace/indent] [4] That's because the namespace indent rule is fairly new and its hard to fix without touching the whole file. I don't think the style-queue should be spamming bugs with non-actionable information. There seem to be a few choices: 1) Disable this warning because it's not helpful at the moment. 2) Change our code to comply with this style rule (e.g., as we touch files, fix them so that the warning doesn't occur). 3) Change our style guide to match our code. Personally, I'm in favor of (2) because it seems silly to have a rule in our style guide with which we never plan to actually comply. Thoughts? Adam ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] namespace indent
4) (I think it may be possible to) notice that there are other unchanged lines that have this problem, and then just not print the error if that occurs. dave On Tue, Dec 1, 2009 at 12:42 PM, Adam Barth aba...@webkit.org wrote: A common false positive in the style-queue is as follows: Code inside a namespace should not be indented. [whitespace/indent] [4] That's because the namespace indent rule is fairly new and its hard to fix without touching the whole file. I don't think the style-queue should be spamming bugs with non-actionable information. There seem to be a few choices: 1) Disable this warning because it's not helpful at the moment. 2) Change our code to comply with this style rule (e.g., as we touch files, fix them so that the warning doesn't occur). 3) Change our style guide to match our code. Personally, I'm in favor of (2) because it seems silly to have a rule in our style guide with which we never plan to actually comply. Thoughts? Adam ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] namespace indent
Adam's right though: unlike most of the other style changes, if we don't fix this one all at once, only new files will ever match the style guide. This is different than most of the other guidelines where things eventually converge as people touch lines of the file. On Tue, Dec 1, 2009 at 3:22 PM, David Levin le...@chromium.org wrote: 4) (I think it may be possible to) notice that there are other unchanged lines that have this problem, and then just not print the error if that occurs. dave On Tue, Dec 1, 2009 at 12:42 PM, Adam Barth aba...@webkit.org wrote: A common false positive in the style-queue is as follows: Code inside a namespace should not be indented. [whitespace/indent] [4] That's because the namespace indent rule is fairly new and its hard to fix without touching the whole file. I don't think the style-queue should be spamming bugs with non-actionable information. There seem to be a few choices: 1) Disable this warning because it's not helpful at the moment. 2) Change our code to comply with this style rule (e.g., as we touch files, fix them so that the warning doesn't occur). 3) Change our style guide to match our code. Personally, I'm in favor of (2) because it seems silly to have a rule in our style guide with which we never plan to actually comply. Thoughts? Adam ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] namespace indent
On Tue, Dec 1, Jeremy Orlow wrote: Adam's right though: unlike most of the other style changes, if we don't fix this one all at once, only new files will ever match the style guide. This is different than most of the other guidelines where things eventually converge as people touch lines of the file. I am in favor of using a script to do this change. It looks like we're using a similar approach to do a global rename: http://trac.webkit.org/browser/trunk/WebKitTools/Scripts/do-webcore-rename Once created, the script could be executed on a per folder, or per project basis, etc. depending on what people prefer. I expressed interest a couple weeks ago in writing such a script: https://lists.webkit.org/pipermail/webkit-dev/2009-November/010521.html Since I don't have a time frame though, I'm not suggesting that people hold off on fixing the issue in files they touch. --Chris On Tue, Dec 1, 2009 at 3:22 PM, David Levin levin at chromium.org wrote: 4) (I think it may be possible to) notice that there are other unchanged lines that have this problem, and then just not print the error if that occurs. dave ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
