1318282 - Convert some code in storage/ to C++11

Status: RESOLVED FIXED

(Developer Infrastructure :: Source Code Analysis, defect)

Description

[Andi [:andi]]

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 1

[Andi [:andi]]

Attachment: Bug 1318282 - Converts for(...; ...; ...) loops to use the new range-based loops in C++11 in storage/.

Review commit: https://reviewboard.mozilla.org/r/93696/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/93696/

Comment 2

[Andi [:andi]]

Attachment: Bug 1318282 - Replace default bodies of special member functions with = default; in storage/.

Review commit: https://reviewboard.mozilla.org/r/93698/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/93698/

Comment 3

[Andi [:andi]]

Attachment: Bug 1318282 - Replace string literals containing escaped characters with raw string literals in storage/.

Review commit: https://reviewboard.mozilla.org/r/93700/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/93700/

Comment 4

[Andi [:andi]]

Attachment: Bug 1318282 - Use C++11's override and remove virtual where applicable in storage/.

Review commit: https://reviewboard.mozilla.org/r/93702/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/93702/

Comment 5

[Marco Bonardo [:mak]]

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&".

Comment 6

[Marco Bonardo [:mak]]

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

Comment 7

[Marco Bonardo [:mak]]

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

Comment 8

[Marco Bonardo [:mak]]

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.

Comment 9

[Masatoshi Kimura [:emk]]

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!"

Comment 10

[Andi [:andi]]

(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 11

[Andi [:andi]]

The result from treeherder: https://treeherder.mozilla.org/#/jobs?repo=try&revision=212505e14dce138b3bf8968fd1713ac2c52874f1

Comment 12

[Andi [:andi]]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/93696/diff/1-2/

Comment 13

[Andi [:andi]]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/93698/diff/1-2/

Comment 14

[Andi [:andi]]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/93700/diff/1-2/

Comment 15

[Andi [:andi]]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/93702/diff/1-2/

Comment 16

[Pulsebot]

Pushed by [e-mail]:
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

[Phil Ringnalda (:philor)]

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

Comment 18

[Masatoshi Kimura [:emk]]

(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?

Comment 19

[Andi [:andi]]

(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.