Closed Bug 1510467 Opened 6 years ago Closed 6 years ago

Preserve purposely-formatted code from clang-format

Categories

(Core :: Graphics: CanvasWebGL, defect, P1)

defect

Tracking

()

RESOLVED FIXED
mozilla65
Tracking Status
firefox-esr60 --- fixed
firefox65 --- fixed

People

(Reporter: jgilbert, Assigned: jgilbert)

References

(Blocks 1 open bug)

Details

(Whiteboard: gfx-noted)

Attachments

(3 files)

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)
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)
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)
Flags: needinfo?(ehsan)
Attachment #9028148 - Flags: review?(ehsan)
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla65
(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 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
Attached patch ESR patch (part 1) (deleted) — Splinter Review
[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?
Attached patch ESR patch (part 2) (deleted) — Splinter Review
Attachment #9031017 - Flags: approval-mozilla-esr60?
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+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: