[Webkit-unassigned] [Bug 27461] Add checks for namespace indentation to cpplint

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=27461





--- Comment #8 from Jakob Petsovits <jpetso at gmx.at>  2009-07-21 08:27:22 PDT ---
Thanks for the detailed review! Here's a new patch addressing the points from
comments #6.

> ...
> > +    line = clean_lines.elided[line_number]        # get rid of comments and strings
>
> Just one space before the #.

Actually that line was copied from the function below, so the updated patch
fixes both my line and the original one.

> > +            # Skip not only empty lines but also those with preprocessor directives.
>
> What about labels? (Not case labels but other labels.)

Good point, I added a check for those too - if they're at the start of a line,
otherwise that's just weird, or a switch label. As the loop operates on raw
lines (in order to catch indentation of comments as well), I didn't match until
the end of the string "$" but only until the colon, hopefully it'll be ok this
way. If not, I will need to loop over both raw and elided lines in parallel.

> btw, I wonder if this functionality would be better as something which
> processed the whole file at once (like the functions called from
> process_file_data()).
>
> If written that way, I could see this function more easily morphing (in the
> future) into something which verified the indent of every line.  (Also it just
> feels more natural given that it loops over all of the lines after it hits a
> namespace.)
>
> What do you think about this?

It might make sense in the long term if we want to build in further, more
detailed checks. But checking the indent of every line also has a big downside,
as such a "continuous" check needs to keep track of indentation state
throughout all the lines that it checks. For example, my current patch avoids
this situation for indentation files by only checking the first line after the
namespace opens, because detecting the actual ending brace for the namespace is
way over my head without some kind of at least a half-decent C++ parser. With
an all-at-once approach, this kind of "hack" is not an option.

We'd also need to build in manual nesting support (e.g. for nested namespaces,
or switching from a namespace context to a class context and back) which the
current check can avoid because it checks each context independently from each
other.

So while an all-at-once approach has the potential to improve the quality of
indentation checks in general, I think it's a larger undertaking and might
necessitate more complexity than what's in scope of this patch. imho.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.