Closed Bug 1064620 Opened 6 years ago Closed 5 years ago

Considering changing preprocessing marker from # to //# in mobile/android Java code

Categories

(Firefox Build System :: Android Studio and Gradle Integration, defect)

All
Android
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
mozilla35

People

(Reporter: nalexander, Assigned: ckitching)

Details

Attachments

(1 file, 2 obsolete files)

We would make the lives of IDE users simpler if we made our .java.in code syntactically valid.  I don't see a way to make @...@ work, since /*@...@*/ would be missing a token, but there's little to lose from making our preprocessor marker //# instead of #.

In this particular example, ckitching reports that Android Studio fails hard on #substitution on the first line of .java.in files.
ckitching: you could finish this off and see if it makes your life
better.  Comments on the ticket appreciated.

I think I'd want feedback from mobile-firefox-dev before landing this.
In particular, the platform crew (blassey, snorp, nchen) might not
appreciate the context switch between Java and C++ code.
Attachment #8486113 - Flags: feedback?(chriskitching)
Applied the pattern to all java.in files. This has made a real improvement: IDEA can finally resolve AppConstants references, among other things (some minor brokenness within the changes files where there are missing values, but that's not really a problem.
Attachment #8486120 - Flags: review?(nalexander)
Assignee: nobody → chriskitching
Status: NEW → ASSIGNED
Attachment #8486113 - Attachment is obsolete: true
Attachment #8486113 - Flags: feedback?(chriskitching)
Comment on attachment 8486120 [details] [diff] [review]
Replace # with //# for IDE integration

I have no doubt the patch is correct but will need to be updated for landing.  This is blocked on consensus, not on technical correctness.  We'll reflag when we have consensus.
Attachment #8486120 - Flags: review?(nalexander)
Attachment #8486120 - Attachment is obsolete: true
(Now with more qref: this one actually builds)

*whistles innocently*
*pokes with a stick*

How do we perform the required politics to make this land?

... Does anyone have a notion of what the arguments against doing this are? It feels like we just stalled right before doing a really neat thing for an intangible reason: "it might annoy someone".
(In reply to Chris Kitching [:ckitching] from comment #6)
> How do we perform the required politics to make this land?

I'd say mail the list and if no one objects after a few days, land it. If they object afterwards, they can comment in the bug and it can later be backed out.

> ... Does anyone have a notion of what the arguments against doing this are?

Perhaps:
  * Less visible because syntax highlighters (sometimes) recognize `#`, but will just mark `//#` as a comment.
  * Less visible if the comments are indented, as comments usually are, instead of at column 1 (I would say don't indent)
  * Scripts may pick up pre-processing with a `^#` regex so you might break some things (seems to be an easy fix though) - I doubt anything important does this though
Comment on attachment 8486897 [details] [diff] [review]
Replace # with //# for IDE integration

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

Since nobody appears to care, and it makes Nick's Gradle-based IDEA support very slightly less ridiculously awesome than it already is, I can haz review to land this?
Attachment #8486897 - Flags: review?(rnewman)
Comment on attachment 8486897 [details] [diff] [review]
Replace # with //# for IDE integration

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

Rubberstamp. Ping Nick if you want a real build opinion!
Attachment #8486897 - Flags: review?(rnewman) → review+
Learning the hard way that ^Cing a push is a bad plan...

Looks like someone sneaked an extra few preprocessor directives into AppConstants since the patch was last checked. It didn't conflict with the other chunks but caused asplosions. Pushing without building was a stupid thing to try to do :/ .

I'll land the fixed patch when the tree opens.
Sorry for making merge weekend more "exciting"!
https://hg.mozilla.org/mozilla-central/rev/b557f2cb4a04
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 35
Product: Firefox for Android → Firefox Build System
Target Milestone: Firefox 35 → mozilla35
You need to log in before you can comment on or make changes to this bug.