Convert some code in storage/ to C++11

RESOLVED FIXED in Firefox 53

Status

Firefox Build System
Source Code Analysis
RESOLVED FIXED
2 years ago
5 months ago

People

(Reporter: andi, Assigned: andi)

Tracking

(Blocks: 1 bug)

Trunk
mozilla53

Firefox Tracking Flags

(firefox53 fixed)

Details

MozReview Requests

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(4 attachments)

(Assignee)

Description

2 years ago
Using the checkers provided by clang-tidy we can refresh the code to make it use the features of C++11 like:

- auto variables declarations
- default CTORS and DTORS
- using new range loop operators
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

2 years ago
Attachment #8811671 - Flags: review?(mak77)
Attachment #8811672 - Flags: review?(mak77)
Attachment #8811673 - Flags: review?(mak77)
Attachment #8811674 - Flags: review?(mak77)

Comment 5

2 years ago
mozreview-review
Comment on attachment 8811671 [details]
Bug 1318282 - Converts for(...; ...; ...) loops to use the new range-based loops in C++11 in storage/.

https://reviewboard.mozilla.org/r/93696/#review94570

::: storage/mozStorageConnection.cpp:1984
(Diff revision 1)
>  NS_IMETHODIMP
>  Connection::EnableModule(const nsACString& aModuleName)
>  {
>    if (!mDBConn) return NS_ERROR_NOT_INITIALIZED;
>  
> -  for (size_t i = 0; i < ArrayLength(gModules); i++) {
> +  for (auto & gModule : gModules) {

this one has a whitspace between auto and &, while the previous one doesn't. I don't know whether we decided a platform wide style but at least they should be consistent.

I'd say & towards type for Storage, so use "auto&".
Attachment #8811671 - Flags: review?(mak77) → review+

Comment 6

2 years ago
mozreview-review
Comment on attachment 8811672 [details]
Bug 1318282 - Replace default bodies of special member functions with = default; in storage/.

https://reviewboard.mozilla.org/r/93698/#review94608
Attachment #8811672 - Flags: review?(mak77) → review+

Comment 7

2 years ago
mozreview-review
Comment on attachment 8811673 [details]
Bug 1318282 - Replace string literals containing escaped characters with raw string literals in storage/.

https://reviewboard.mozilla.org/r/93700/#review94610
Attachment #8811673 - Flags: review?(mak77) → review+

Comment 8

2 years ago
mozreview-review
Comment on attachment 8811674 [details]
Bug 1318282 - Use C++11's override and remove virtual where applicable in storage/.

https://reviewboard.mozilla.org/r/93702/#review94616

::: storage/test/test_AsXXX_helpers.cpp:24
(Diff revision 1)
>  public:
>    NS_DECL_ISUPPORTS_INHERITED
>    NS_DECL_ASYNCSTATEMENTSPINNER
>    Spinner() {}
>  protected:
> -  virtual ~Spinner() = default;
> +  ~Spinner() override = default;

bug 1315138 may end up undoing this change, please notify it there, so it ccan be ported to the new test.
Attachment #8811674 - Flags: review?(mak77) → review+

Comment 9

2 years ago
mozreview-review
Comment on attachment 8811673 [details]
Bug 1318282 - Replace string literals containing escaped characters with raw string literals in storage/.

https://reviewboard.mozilla.org/r/93700/#review94800

::: storage/test/test_binding_params.cpp:129
(Diff revision 1)
>      "SELECT str FROM test"
>    ), getter_AddRefs(select));
>  
>    // Roundtrip a UTF8 string through the table, using UTF8 input and output.
>    static const char sCharArray[] =
> -    "I'm a \xc3\xbb\xc3\xbc\xc3\xa2\xc3\xa4\xc3\xa7 UTF8 string!";
> +    R"(I'm a ûüâäç UTF8 string!)";

Why is the raw string literal needed here?

Also, is this conversion correct? It will depend on source character set and execution character set although I set it "UTF-8" in bug 1313280. Does clang-tidy disregard other compilers? (Clang will always use UTF-8 for both character sets.)

It would be more portable to use UTF-8 string literal and universal character name:
  u"I'm a \u00fb\u00fc\u00e2\u00e4\u00e7 UTF8 string!"
(Assignee)

Comment 10

2 years ago
(In reply to Masatoshi Kimura [:emk] from comment #9)
> Comment on attachment 8811673 [details]
> Bug 1318282 - Replace string literals containing escaped characters with raw
> string literals in storage/.
> 
> https://reviewboard.mozilla.org/r/93700/#review94800
> 
> ::: storage/test/test_binding_params.cpp:129
> (Diff revision 1)
> >      "SELECT str FROM test"
> >    ), getter_AddRefs(select));
> >  
> >    // Roundtrip a UTF8 string through the table, using UTF8 input and output.
> >    static const char sCharArray[] =
> > -    "I'm a \xc3\xbb\xc3\xbc\xc3\xa2\xc3\xa4\xc3\xa7 UTF8 string!";
> > +    R"(I'm a ûüâäç UTF8 string!)";
> 
> Why is the raw string literal needed here?
> 
> Also, is this conversion correct? It will depend on source character set and
> execution character set although I set it "UTF-8" in bug 1313280. Does
> clang-tidy disregard other compilers? (Clang will always use UTF-8 for both
> character sets.)
> 
> It would be more portable to use UTF-8 string literal and universal
> character name:
>   u"I'm a \u00fb\u00fc\u00e2\u00e4\u00e7 UTF8 string!"

I see your point but this builds on treeherder on all platforms so i guess other compilers like gcc and visual compiler accepts this kind of encoding. 
Regarding why this change, it's more visible what characters are in the string, this is the whole purpose of raw string literal usage.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 16

2 years ago
Pushed by bpostelnicu@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/8616948dc56c
Converts for(...; ...; ...) loops to use the new range-based loops in C++11 in storage/. r=mak
https://hg.mozilla.org/integration/autoland/rev/760ff7b0570c
Replace default bodies of special member functions with = default; in storage/. r=mak
https://hg.mozilla.org/integration/autoland/rev/82c130ab1511
Replace string literals containing escaped characters with raw string literals in storage/. r=mak
https://hg.mozilla.org/integration/autoland/rev/5e47faec51ed
Use C++11's override and remove virtual where applicable in storage/. r=mak

Comment 17

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/8616948dc56c
https://hg.mozilla.org/mozilla-central/rev/760ff7b0570c
https://hg.mozilla.org/mozilla-central/rev/82c130ab1511
https://hg.mozilla.org/mozilla-central/rev/5e47faec51ed
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox53: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
(In reply to Andi-Bogdan Postelnicu from comment #10)
> I see your point but this builds on treeherder on all platforms so i guess
> other compilers like gcc and visual compiler accepts this kind of encoding. 

I'm OK with assuming that the source encoding is utf-8 in our tree.

> Regarding why this change, it's more visible what characters are in the
> string, this is the whole purpose of raw string literal usage.

If the string needs some escapes, raw string literals would be better. But in this case, the converted string does not contain any escapes at all because all escapes are converted to the raw UTF-8 octets that do not require raw string literals to represent without escape (if we can assume the source encoding is utf-8).

So
  "I'm a ûüâäç UTF8 string!";
would be even more visible. (No extra 'R' prefix and parenthesis.) Thoughts?
Flags: needinfo?(bpostelnicu)
(Assignee)

Comment 19

2 years ago
(In reply to Masatoshi Kimura [:emk] from comment #18)
> (In reply to Andi-Bogdan Postelnicu from comment #10)
> > I see your point but this builds on treeherder on all platforms so i guess
> > other compilers like gcc and visual compiler accepts this kind of encoding. 
> 
> I'm OK with assuming that the source encoding is utf-8 in our tree.
> 
> > Regarding why this change, it's more visible what characters are in the
> > string, this is the whole purpose of raw string literal usage.
> 
> If the string needs some escapes, raw string literals would be better. But
> in this case, the converted string does not contain any escapes at all
> because all escapes are converted to the raw UTF-8 octets that do not
> require raw string literals to represent without escape (if we can assume
> the source encoding is utf-8).
> 
> So
>   "I'm a ûüâäç UTF8 string!";
> would be even more visible. (No extra 'R' prefix and parenthesis.) Thoughts?

I agree with you on this one, i think it's just a matter of preference.
Flags: needinfo?(bpostelnicu)

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.