Last Comment Bug 585099 - header.py should emit adjacent consts in the same enum
: header.py should emit adjacent consts in the same enum
Status: RESOLVED FIXED
[mentor=jdm][lang=python]
:
Product: Core
Classification: Components
Component: Build Config (show other bugs)
: Trunk
: x86 Linux
: -- normal (vote)
: mozilla11
Assigned To: Atul Aggarwal
:
Mentors:
Depends on:
Blocks: 666688
  Show dependency treegraph
 
Reported: 2010-08-06 10:00 PDT by Josh Matthews [:jdm]
Modified: 2012-02-01 13:58 PST (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch (2.68 KB, patch)
2010-08-06 10:00 PDT, Josh Matthews [:jdm]
no flags Details | Diff | Review
Patch (2.69 KB, patch)
2010-08-06 10:03 PDT, Josh Matthews [:jdm]
no flags Details | Diff | Review
Emit adjacent consts in IDL files in the same enum in generated headers. (2.70 KB, patch)
2011-09-04 09:19 PDT, Josh Matthews [:jdm]
ted: review-
khuey: review+
Details | Diff | Review
Patch v1 (3.79 KB, patch)
2011-11-08 12:23 PST, Atul Aggarwal
ted: review-
Details | Diff | Review
Patch v1.01 (3.87 KB, patch)
2011-11-16 04:58 PST, Atul Aggarwal
ted: review+
Details | Diff | Review
Patch v1.10 (3.93 KB, patch)
2011-11-16 09:59 PST, Atul Aggarwal
ted: review+
Details | Diff | Review

Description Josh Matthews [:jdm] 2010-08-06 10:00:35 PDT
Created attachment 463578 [details] [diff] [review]
Patch

Analogous to bug 262000, this helps get rid of warnings of comparisons against anonymous enums.  I haven't figured out how to run header.py to test my changes yet, however.
Comment 1 Josh Matthews [:jdm] 2010-08-06 10:03:42 PDT
Created attachment 463581 [details] [diff] [review]
Patch

Now with a higher chance of working.
Comment 2 Josh Matthews [:jdm] 2011-09-04 09:19:04 PDT
Created attachment 558158 [details] [diff] [review]
Emit adjacent consts in IDL files in the same enum in generated headers.

Output of nsITypeAheadFind.idl with this patch:
  enum { FIND_FOUND = 0U,
         FIND_NOTFOUND = 1U,
         FIND_WRAPPED = 2U };
Comment 3 Kyle Huey [:khuey] (khuey@mozilla.com) 2011-09-06 09:01:44 PDT
Do you know why the binary xpidl did *not* do this?
Comment 4 Josh Matthews [:jdm] 2011-09-06 09:11:31 PDT
Because dbradley never got around to looking into my patch in bug 262000. The discussion is all there, and he was all for the idea on IRC when I chatted with him.
Comment 5 Benjamin Smedberg [:bsmedberg] 2011-09-06 09:38:43 PDT
Does WebIDL have anything interesting to say about these consts? Long-term I'd really prefer to just have enums in IDL, either anonymous or named, and have as little magic as possible. But I'm not going to oppose the short-term improvement here.
Comment 6 Kyle Huey [:khuey] (khuey@mozilla.com) 2011-09-06 09:40:35 PDT
WebIDL does not have enums.
Comment 7 Kyle Huey [:khuey] (khuey@mozilla.com) 2011-09-07 04:26:13 PDT
Comment on attachment 558158 [details] [diff] [review]
Emit adjacent consts in IDL files in the same enum in generated headers.

This looks fine to me, but I'm going to let ted review it since I imagine there's a cleaner more-pythonic way to do this.
Comment 8 Ted Mielczarek [:ted.mielczarek] 2011-09-23 12:06:47 PDT
This patch looks like it will work, but it's a little messy. I'm trying to suggest an alternate way of doing it, but I haven't come up with anything useful yet.
Comment 9 Ted Mielczarek [:ted.mielczarek] 2011-09-28 05:47:11 PDT
Comment on attachment 558158 [details] [diff] [review]
Emit adjacent consts in IDL files in the same enum in generated headers.

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

Okay, here's my alternate implementation suggestion.
The itertools module has a neat function called groupby:
http://docs.python.org/library/itertools.html#itertools.groupby

That will return a set of iterators grouping an input iterator by a key function you provide. If you use 'type' as the key function, then you'll get them grouped by type, so your xpidl.ConstMembers will be grouped for you (as well as everything else, but that's trivial to work with). The code would look something like this:

for key, group in itertools.groupby(iface.members, key=type):
    if key == xpidl.ConstMember:
        write_const_decls(group) # group is an iterator over the adjacent consts here
    else:
      for member in group:
          # use the existing loop logic here to write members out one-at-a-time

I think the loop logic here looks much cleaner, and the logic in write_const_decls would wind up really simple and clean as well. How does that sound?
Comment 10 Josh Matthews [:jdm] 2011-10-01 23:50:26 PDT
It would be great for someone with a desire to write some python could address Ted's suggestions in comment 9.
Comment 11 Atul Aggarwal 2011-11-08 12:23:28 PST
Created attachment 572944 [details] [diff] [review]
Patch v1

ted, you wrote almost everything in your last comment.. Copied it in my patch :). BTW, this is my first meaningful python editing.
Comment 12 Atul Aggarwal 2011-11-08 12:25:05 PST
https://tbpl.mozilla.org/?tree=Try&rev=887274afcb98 to make sure nothing broke with my change.
Comment 13 Ted Mielczarek [:ted.mielczarek] 2011-11-10 13:13:11 PST
Comment on attachment 572944 [details] [diff] [review]
Patch v1

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

The method looks okay, but r- for a few no-nos.

::: xpcom/idl-parser/header.py
@@ +334,5 @@
> +            printComments(fd, c.doccomments, '  ')
> +            basetype = c.basetype
> +            value = c.getValue()
> +            s += "\n    " + c.name + " = " + str(value) + \
> +                 ("U" if (not basetype.signed) else "") + ",\n"

Please don't use string concatenation like this. Use string formatting like the code you're replacing.

@@ +335,5 @@
> +            basetype = c.basetype
> +            value = c.getValue()
> +            s += "\n    " + c.name + " = " + str(value) + \
> +                 ("U" if (not basetype.signed) else "") + ",\n"
> +        s = s[:-2] +"\n  };\n\n"

The [:-2] bit is ugly. You should either push these all into a list, and then use "\n".join() on them, or just deal with the extra newline, since it's in a generated source file anyway.
Comment 14 Atul Aggarwal 2011-11-16 04:58:18 PST
Created attachment 574875 [details] [diff] [review]
Patch v1.01

changed as suggested by ted.

After this change, enum will be displayed as below (note extra comma in the last enum also). If this is not acceptable, please let me know.

   enum {
     FIND_FOUND = 0U,
     FIND_NOTFOUND = 1U,
     FIND_WRAPPED = 2U,
   };
Comment 15 Ted Mielczarek [:ted.mielczarek] 2011-11-16 08:01:57 PST
Comment on attachment 574875 [details] [diff] [review]
Patch v1.01

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

::: xpcom/idl-parser/header.py
@@ +336,5 @@
> +            value = c.getValue()
> +            fd.write("\n    %(name)s = %(value)s%(signed)s," % {
> +                         'name': c.name,
> +                         'value': value,
> +                         'signed': (not basetype.signed) and 'U' or ''})

I think the trailing comma is probably going to break GCC, since we compile with -pedantic. I can throw this at the try server to be sure, or you can just tweak this to append all the entries to a list, and just ",\n".join() the list at the end.
Comment 16 Atul Aggarwal 2011-11-16 09:15:07 PST
I was able to compile it using gcc4.6. I also pushed it to try server to be sure (https://tbpl.mozilla.org/?tree=Try&rev=e5a47cfad830).

However if ",\n".join looks more elegant, I could also pursue it :)
Comment 17 Ted Mielczarek [:ted.mielczarek] 2011-11-16 09:51:58 PST
Comment on attachment 574875 [details] [diff] [review]
Patch v1.01

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

Okay, great! Thanks for testing that.
Comment 18 Atul Aggarwal 2011-11-16 09:59:26 PST
Created attachment 574926 [details] [diff] [review]
Patch v1.10

Adding join to be more portable across compilers.
Comment 19 Ted Mielczarek [:ted.mielczarek] 2011-11-16 10:02:20 PST
Comment on attachment 574926 [details] [diff] [review]
Patch v1.10

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

Thanks!
Comment 21 Ed Morley [:emorley] 2011-11-20 14:22:59 PST
https://hg.mozilla.org/mozilla-central/rev/16073086d49b

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