Bug 27557

Summary:

Case-insensitive comparison of include file sorting for cpplint

Product:

WebKit

Reporter:

Jakob Petsovits <jpetsovits>

Component:

Tools / Tests

Assignee:

Nobody <webkit-unassigned>

Status:

RESOLVED WONTFIX

Severity:

Normal

CC:

darin, manyoso, mitz, staikos

Priority:

Version:

528+ (Nightly build)

Hardware:

OS:

All

Attachments:

Description	Flags
Case-insensitive comparison of include file order for cpplint	darin: review-

Jakob Petsovits

Reported 2009-07-22 12:58:56 PDT

Quoting my test case: #include "a.h" '#include "B.h" '#include "c.h" That thing currently fails. The fix is straightforward, patch goes below.

Attachments
Case-insensitive comparison of include file order for cpplint (2.91 KB, patch) 2009-07-22 13:01 PDT, Jakob Petsovits	darin: review-	Details Formatted Diff Diff
View All Add attachment proposed patch, testcase, etc.

Jakob Petsovits

Comment 1 2009-07-22 13:01:06 PDT

Created attachment 33284 [details] Case-insensitive comparison of include file order for cpplint

Darin Adler

Comment 2 2009-07-22 13:02:42 PDT

Comment on attachment 33284 [details] Case-insensitive comparison of include file order for cpplint The order is supposed to be case-sensitive, as if done by the command line "sort" tool. Did someone say it should be case-insensitive?

Darin Adler

Comment 3 2009-07-22 13:05:01 PDT

Comment on attachment 33284 [details] Case-insensitive comparison of include file order for cpplint Setting to review- for now. I believe the sort is supposed to be case sensitive, but if there is consensus otherwise, then this is a good change to make.

Adam Treat

Comment 4 2009-07-22 13:06:47 PDT

Oops. I committed it already. Should I back out until we find consensus?

Darin Adler

Comment 5 2009-07-22 13:15:45 PDT

(In reply to comment #4) > Oops. I committed it already. Should I back out until we find consensus? Yes, I think so. I'm almost certain our standard was case-sensitive as done by the "sort" tool, and I believe it would require discussion to change that.

Jakob Petsovits

Comment 6 2009-07-22 13:19:45 PDT

Then I'm probably adding a test case to clarify that way of working, right?

Adam Treat

Comment 7 2009-07-22 13:22:46 PDT

Reverted with r46235. Sorry guys. It just doesn't seem very intuitive to me to sort alphabetically with case-sensitivity. But if thems the rules, thems the rules.

George Staikos

Comment 8 2009-07-22 13:31:40 PDT

sort -f? All in all the concept of sorting headers alphabetically as a rule seems like a scary and arbitrary decision, though I'm really not interested in debating it.

Darin Adler

Comment 9 2009-07-22 13:34:34 PDT

Why do we sort at all? One reason is to make it less likely we will end up with duplicate includes. Another is to ensure we don't get a variety of different "logical" sorting orders invented by different well-meaning programmers in different source files. Case-sensitive vs. case-insensitive is entirely arbitrary, and we chose case-sensitive long ago. I'd be open to considering a change.

Note You need to log in before you can comment on or make changes to this bug.