[clang-format] Space should separate braces in empty code

RESOLVED WONTFIX

Status

Firefox Build System
Source Code Analysis
RESOLVED WONTFIX
a year ago
5 months ago

People

(Reporter: jya, Assigned: andi)

Tracking

(Blocks: 1 bug)

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

a year ago
Per coding style rules:

https://developer.mozilla.org/fr/docs/Developer_Guide/Coding_Style#Classes

  // Tiny constructors and destructors can be written on a single line.
  MyClass() { }


And not:
  // Tiny constructors and destructors can be written on a single line.
  MyClass() {}

There should be a space for empty bodied code
(Assignee)

Updated

a year ago
Assignee: nobody → bpostelnicu
(Assignee)

Comment 1

a year ago
I'm looking over this issue but i think this could be simplified a bit. 

1. Destructor
   For empty body destructors i think the best approach would be to use the default directive

2. Constructor
   For empty constructors that don't have neither initialisation list nor body we should also adopt the default directive.
   For constructor that only have initialiser list we should use the default brace placement.
Comment hidden (mozreview-request)
(Reporter)

Updated

a year ago
Attachment #8845827 - Attachment is obsolete: true
Attachment #8845827 - Flags: review?(jwwang)
(Reporter)

Updated

a year ago
Attachment #8845827 - Attachment is obsolete: false
(Reporter)

Updated

a year ago
Attachment #8845827 - Attachment is obsolete: true
(Reporter)

Updated

10 months ago
See Also: → bug 1399359
(Reporter)

Updated

10 months ago
See Also: → bug 1399087
Blocks: 1188202
(Assignee)

Comment 3

10 months ago
Created attachment 8909327 [details] [diff] [review]
patch.diff

I've created this patch against clang repo in order to have support for space in empty functions that are not split on two lines.
I've tried it on some files and it produces the desired behaviour.
When you have time please try it and tell me if you think it's OK.
Attachment #8909327 - Flags: feedback?(jyavenard)
(Assignee)

Comment 4

10 months ago
Comment on attachment 8909327 [details] [diff] [review]
patch.diff

I've cancelled the feedback request since I will directly submit this to upstream and I don't want to burden you with the build of clang-tidy.
Attachment #8909327 - Flags: feedback?(jyavenard) → feedback-
(Assignee)

Comment 5

10 months ago
(In reply to Andi-Bogdan Postelnicu [:andi] from comment #4)
> Comment on attachment 8909327 [details] [diff] [review]
> patch.diff
> 
> I've cancelled the feedback request since I will directly submit this to
> upstream and I don't want to burden you with the build of clang-tidy.
Minor correct, clang-format not clang-tidy.
(Assignee)

Comment 6

10 months ago
Has been submitted upstream: https://reviews.llvm.org/D37979
(Assignee)

Updated

10 months ago
Attachment #8909327 - Flags: feedback-
(Assignee)

Comment 7

9 months ago
As per the resolution from https://reviews.llvm.org/D37979, the patch has not been accepted so we must change our coding style i guess.
Status: NEW → RESOLVED
Last Resolved: 9 months ago
Resolution: --- → WONTFIX

Updated

5 months ago
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.