Closed Bug 1429212 Opened 2 years ago Closed 2 years ago

Add --fixup option to check_spidermonkey_style.py

Categories

(Core :: JavaScript Engine, enhancement, P3)

enhancement

Tracking

()

RESOLVED FIXED
mozilla60
Tracking Status
firefox60 --- fixed

People

(Reporter: jorendorff, Assigned: jorendorff)

Details

Attachments

(1 file)

Another random chunk of code that I wrote, and found handy.

It does not fix all possible style problems that the script checks. It just sorts sequences of #include directives. It modifies your source files in-place.
Assignee: nobody → jorendorff
Status: NEW → ASSIGNED
Comment on attachment 8941202 [details] [diff] [review]
Add --fixup mode to check_spidermonkey_style.py

Review of attachment 8941202 [details] [diff] [review]:
-----------------------------------------------------------------

::: config/check_spidermonkey_style.py
@@ +431,5 @@
> +        self.kids[-1].lines.append(line)
> +
> +    def style_relevant_kids(self):
> +        """ Return a list of kids in this block that are style-relevant. """
> +        return [kid for kid in self.kids if kid.is_style_relevant()]

Please implement a version of babel for Python and rewrite this as

    return filter(kid => kid.is_style_relevant(), self.kids)

or maybe even

    return self.kids.filter(kid => kid.is_style_relevant())

(if rewriting babel is not on your roadmap, leave it as is; Python lambdas are ugly.)

@@ +446,5 @@
> +            decorated = sorted(zip(keys, includes))
> +
> +            output = []
> +            current_section = None
> +            for (section, _), inc in decorated:

I think this is a little clearer without the 'decorated' temporary. Maybe because I didn't initially understand what "decorated" meant here.

@@ +467,5 @@
> +                if last_key is not None:
> +                    if last_key > key:
> +                        is_sorted = False
> +                last_key = key
> +            return is_sorted

I find this a little hard to follow. What do you think of

    if any(inc.inclname in oddly_ordered_inclnames for inc in includes):
        return True
    return includes == sorted(includes, key=lambda inc: inc.sort_key(enclosing_inclnames))

(or if that's too dense, at least rename last_key to prev_key.)

Except I don't understand the logic. It seems like this accepts any ordering as long as you include at least one oddly_ordered_inclname in the list. Shouldn't this rather be:

     relevant_includes = [inc for inc in includes if inc.inclname not in oddly_ordered_inclnames]
     return relevant_includes == sorted(relevant_includes, key=lambda inc: inc.sort_key(enclosing_inclname))

?

@@ +472,5 @@
> +
> +        output = []
> +        batch = []  # list of Include objects and whitespace OrdinaryCode objects
> +
> +        def flush():

This function needs comments. Especially since it seems to be dropping OrdinaryCode that is mixed into the includes, but not after? Unless is_already_sorted_or_unsortable, in which case it doesn't drop anything?

And the expected input is a plain series of Includes and OrdinaryCode, I guess? Because this would do weird stuff if there were a HashIfBlock in there. Good to comment.

I also wonder if it should be called flush_batch, to call out the side-effecting happening. But I'm ambivalent about that. Closures are not so unexpected.

@@ +485,5 @@
> +            if is_already_sorted_or_unsortable(includes):
> +                output.extend(batch)
> +            else:
> +                output.extend(pretty_sorted_includes(includes) + batch[cutoff:])
> +            del batch[:]

I didn't know del did this. I've always done batch[:] = [].

@@ +499,5 @@
> +                if kid.to_source().isspace():
> +                    batch.append(kid)
> +                else:
> +                    flush()
> +                    output.append(kid)

Huh. I guess I should look at what this file tests. It seems like this would accept

    #include <file2.h>
    extern int ForgetAboutOrdering;
    #include <file1.h>

without reordering. Is that intended? Or is it just a simplification for --fixup?

@@ +546,1 @@
>          if m is not None:

Can you name your captures? Either

    (prefix, name, suffix) = m.groups()

or if you don't mind cluttering your regexp

   r'(?P<prefix>\s*#\s*include\s+)"(?P<name>[^"]*)"(?P<suffix>.*)'

Bleh. I've never written one of those out before. Without whitespace, like Perl's /x modifier, that looks awful.

@@ +546,2 @@
>          if m is not None:
> +            block_stack[-1].kids.append(Include(m.group(1), m.group(2), m.group(3), linenum, False))

Can you make that last argument is_system=False? It's a little cryptic otherwise. Same for True later.
Attachment #8941202 - Flags: review?(sphink) → review+
Priority: -- → P3
Thanks for the great review. I took all the changes and more...

(In reply to Steve Fink [:sfink] [:s:] from comment #2)
> Except I don't understand the logic. It seems like this accepts any ordering
> as long as you include at least one oddly_ordered_inclname in the list.
> Shouldn't this rather be:
> 
>      relevant_includes = [inc for inc in includes if inc.inclname not in
> oddly_ordered_inclnames]
>      return relevant_includes == sorted(relevant_includes, key=lambda inc:
> inc.sort_key(enclosing_inclname))
> 
> ?

Oh. I made a mess here. The method is intended to change code that needs a particular, obvious fixup; and when in doubt, leave the code unchanged, even if that means the resulting file wouldn't pass muster. So I explained that in the docstring.

This particular function's job is to decide whether to bother rearranging the current batch, so I changed it to look like this:

        def should_try_to_sort(includes):
            if 'tests/style/BadIncludes' in enclosing_inclname:
                return False  # don't straighten the counterexample
            if any(inc.inclname in oddly_ordered_inclnames for inc in includes):
                return False  # don't sort batches containing odd includes
            if includes == sorted(includes, key=lambda inc: inc.sort_key(enclosing_inclname)):
                return False  # it's already sorted, avoid whitespace-only fixups
            return True

> This function needs comments. Especially since it seems to be dropping
> OrdinaryCode that is mixed into the includes, but not after? Unless
> is_already_sorted_or_unsortable, in which case it doesn't drop anything?

Right, any OrdinaryCode objects in there are just blank lines. The fixup ignores those blank lines and inserts its own in the places where it likes to see them.

I added an assert to flush_batch to check the invariant.

> I also wonder if it should be called flush_batch, to call out the
> side-effecting happening. [...]

I really liked this idea, so I changed it.

> Huh. I guess I should look at what this file tests. It seems like this would
> accept
> 
>     #include <file2.h>
>     extern int ForgetAboutOrdering;
>     #include <file1.h>
> 
> without reordering. Is that intended? Or is it just a simplification for
> --fixup?

Uh... I'll go with simplification?

> 
> @@ +546,1 @@
> >          if m is not None:
> 
> Can you name your captures? Either
> 
>     (prefix, name, suffix) = m.groups()
> 
> or if you don't mind cluttering your regexp
> 
>    r'(?P<prefix>\s*#\s*include\s+)"(?P<name>[^"]*)"(?P<suffix>.*)'
> 
> Bleh. I've never written one of those out before. Without whitespace, like
> Perl's /x modifier, that looks awful.

In Python, it's written `(?x)`, and it goes at the beginning of the string.

I went with m.groups() instead.
https://hg.mozilla.org/mozilla-central/rev/8bb8cd8cd691
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
You need to log in before you can comment on or make changes to this bug.