Closed
Bug 1510467
Opened 6 years ago
Closed 6 years ago
Preserve purposely-formatted code from clang-format
Categories
(Core :: Graphics: CanvasWebGL, defect, P1)
Core
Graphics: CanvasWebGL
Tracking
()
RESOLVED
FIXED
mozilla65
People
(Reporter: jgilbert, Assigned: jgilbert)
References
(Blocks 1 open bug)
Details
(Whiteboard: gfx-noted)
Attachments
(3 files)
(deleted),
patch
|
ehsan.akhgari
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
lizzard
:
approval-mozilla-esr60+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
lizzard
:
approval-mozilla-esr60+
|
Details | Diff | Splinter Review |
No description provided.
Pushed by jgilbert@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2d672bed9a1d
Preserve purposely-formatted code from clang-format. (webgl, gfx/gl)
Comment 2•6 years ago
|
||
Hi Jeff,
Was there a reason why you decided to push this without the patch being reviewed? Based on a cursory look, it seems like this <https://hg.mozilla.org/integration/mozilla-inbound/rev/2d672bed9a1d#l6.209> should have been |// clang-format on| instead of off, and also the right way to deal with generated code is to add it to .clang-format-ignore to make clang-format skip the file altogether, instead of the GLParseRegistryXML.py change, similar to how we've dealt with other similar examples. Especially in this case since only one file (gfx/gl/GLConsts.h) is affected...
Flags: needinfo?(jgilbert)
Assignee | ||
Comment 3•6 years ago
|
||
I push things without review when review doesn't seem valuable. My go-to reviewers for this would not have known about clang-format-ignore, and I don't expect to receive review from outside my team within the week. The issues you've found here are fairly harmless, so it seems like it worked out.
I'll push up a fix, unless you want to commit to a quick review on that fix?
Flags: needinfo?(jgilbert) → needinfo?(ehsan)
Assignee | ||
Comment 4•6 years ago
|
||
Flags: needinfo?(ehsan)
Attachment #9028148 -
Flags: review?(ehsan)
Comment 5•6 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox65:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla65
Comment 6•6 years ago
|
||
(In reply to Jeff Gilbert [:jgilbert] from comment #3)
> I push things without review when review doesn't seem valuable. My go-to
> reviewers for this would not have known about clang-format-ignore, and I
> don't expect to receive review from outside my team within the week. The
> issues you've found here are fairly harmless, so it seems like it worked out.
Myself and Sylvestre will be happy to do reviews of this nature in the future, and are usually quite responsive (I'd say on average under 24 hours) so there shouldn't be any reason to avoid getting a review next time. Thanks!
> I'll push up a fix, unless you want to commit to a quick review on that fix?
I think the fixes to the above two issues are both quite obvious, so I think you can just push it out. (I'd also be happy to look at it if you prefer, no problem!)
Comment 7•6 years ago
|
||
Comment on attachment 9028148 [details] [diff] [review]
0001-Bug-1510467-Mark-gfx-gl-GLConsts.h-as-generated-in-..patch
Review of attachment 9028148 [details] [diff] [review]:
-----------------------------------------------------------------
Oh I see the patch here, thanks a lot!
Attachment #9028148 -
Flags: review?(ehsan) → review+
Pushed by jgilbert@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/97a481184323
Mark gfx/gl/GLConsts.h as generated in .clang-format-ignore. r=eakhgari
Comment 9•6 years ago
|
||
bugherder |
Comment 10•6 years ago
|
||
[ESR Uplift Approval Request]
If this is not a sec:{high,crit} bug, please state case for ESR consideration: This is required for easier backporting of patches after the reformatting of ESR using clang-format.
User impact if declined: Declining this will negatively impact our developers' ability to easily backport their patches to ESR in the future.
Fix Landed on Version: 65
Risk to taking this patch: Low
Why is the change risky/not risky? (and alternatives if risky): These are comment only changes.
String or UUID changes made by this patch: None
Attachment #9031016 -
Flags: approval-mozilla-esr60?
Comment 11•6 years ago
|
||
Attachment #9031017 -
Flags: approval-mozilla-esr60?
status-firefox-esr60:
--- → affected
Comment on attachment 9031016 [details] [diff] [review]
ESR patch (part 1)
OK for uplift to ESR60 as part of the clang-format project.
Attachment #9031016 -
Flags: approval-mozilla-esr60? → approval-mozilla-esr60+
Attachment #9031017 -
Flags: approval-mozilla-esr60? → approval-mozilla-esr60+
Comment 13•6 years ago
|
||
bugherder uplift |
You need to log in
before you can comment on or make changes to this bug.
Description
•