Closed Bug 1318282 Opened 3 years ago Closed 3 years ago

Convert some code in storage/ to C++11

Categories

(Firefox Build System :: Source Code Analysis, defect)

defect
Not set

Tracking

(firefox53 fixed)

RESOLVED FIXED
mozilla53
Tracking Status
firefox53 --- fixed

People

(Reporter: andi, Assigned: andi)

References

Details

Attachments

(4 files)

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
Attachment #8811671 - Flags: review?(mak77)
Attachment #8811672 - Flags: review?(mak77)
Attachment #8811673 - Flags: review?(mak77)
Attachment #8811674 - Flags: review?(mak77)
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 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 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 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 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!"
(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.
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
(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)
(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)
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.