Closed Bug 1317954 Opened 8 years ago Closed 8 years ago

Convert some code in xpcom/ to C++11

Categories

(Developer Infrastructure :: Source Code Analysis, defect)

Product:
Developer Infrastructure
Component:
Source Code Analysis
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(firefox53 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla53
Tracking Flags:
Tracking Status
firefox53 --- fixed

People

(Reporter: andi, Assigned: andi)

References

Details

Attachments

(5 files)

Bug 1317954 - Converts for(...; ...; ...) loops to use the new range-based loops in C++11 in xpcom/.
58 bytes, text/x-review-board-request
froydnj
: review+
Details
Bug 1317954 - Use auto type specifier where aplicable for variable declarations to improve code readability and maintainability in xpcom/.
58 bytes, text/x-review-board-request
froydnj
: review+
Details
Bug 1317954 - Replace default bodies of special member functions with = default; in xpcom/.
58 bytes, text/x-review-board-request
froydnj
: review+
Details
Bug 1317954 - Replace string literals containing escaped characters with raw string literals in xpcom/.
58 bytes, text/x-review-board-request
froydnj
: review+
Details
Bug 1317954 - Use C++11's override and remove virtual where applicable in xpcom/.
58 bytes, text/x-review-board-request
froydnj
: review+
Details 
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
Summary: Run clang-tidy on xpcom/ module → Convert some code in xpcom/ to C++11
Attachment #8811227 - Flags: review?(nfroyd)
Attachment #8811228 - Flags: review?(nfroyd)
Attachment #8811229 - Flags: review?(nfroyd)
Attachment #8811230 - Flags: review?(nfroyd)
Attachment #8811231 - Flags: review?(nfroyd)
Comment on attachment 8811227 [details]
Bug 1317954 - Converts for(...; ...; ...) loops to use the new range-based loops in C++11 in xpcom/.

https://reviewboard.mozilla.org/r/93418/#review93862

r=me with updates.

::: xpcom/glue/nsID.cpp:16
(Diff revision 2)
> -  for (int i = 0; i < 8; ++i) {
> -    m3[i] = 0;
> +  for (auto& i : m3) {
> +    i = 0;

I'm not thrilled about this one, because I think it makes the code less obvious; why are we modifying the usual loop index in the loop itself?  Replacing this with `memset(&m3[0], 0, sizeof(m3))` would be fine.

::: xpcom/tests/gtest/TestHashtables.cpp:287
(Diff revision 2)
>  TEST(Hashtables, DataHashtable)
>  {
>    // check a data-hashtable
>    nsDataHashtable<nsUint32HashKey,const char*> UniToEntity(ENTITY_COUNT);
>  
> -  for (uint32_t i = 0; i < ENTITY_COUNT; ++i) {
> +  for (auto& gEntitie : gEntities) {

Nit: this should be just `auto& entity : gEntities`.

::: xpcom/tests/gtest/TestHashtables.cpp:293
(Diff revision 2)
> -    UniToEntity.Put(gEntities[i].mUnicode, gEntities[i].mStr);
> +    UniToEntity.Put(gEntitie.mUnicode, gEntitie.mStr);
>    }
>  
>    const char* str;
>  
> -  for (uint32_t i = 0; i < ENTITY_COUNT; ++i) {
> +  for (auto& gEntitie : gEntities) {

Same here.

::: xpcom/tests/gtest/TestHashtables.cpp:320
(Diff revision 2)
>  TEST(Hashtables, ClassHashtable)
>  {
>    // check a class-hashtable
>    nsClassHashtable<nsCStringHashKey,TestUniChar> EntToUniClass(ENTITY_COUNT);
>  
> -  for (uint32_t i = 0; i < ENTITY_COUNT; ++i) {
> +  for (auto& gEntitie : gEntities) {

Same here.

::: xpcom/tests/gtest/TestHashtables.cpp:327
(Diff revision 2)
> -    EntToUniClass.Put(nsDependentCString(gEntities[i].mStr), temp);
> +    EntToUniClass.Put(nsDependentCString(gEntitie.mStr), temp);
>    }
>  
>    TestUniChar* myChar;
>  
> -  for (uint32_t i = 0; i < ENTITY_COUNT; ++i) {
> +  for (auto& gEntitie : gEntities) {

Same here.

::: xpcom/tests/gtest/TestHashtables.cpp:399
(Diff revision 2)
>  TEST(Hashtables, InterfaceHashtable)
>  {
>    // check an interface-hashtable with an uint32_t key
>    nsInterfaceHashtable<nsUint32HashKey,IFoo> UniToEntClass2(ENTITY_COUNT);
>  
> -  for (uint32_t i = 0; i < ENTITY_COUNT; ++i) {
> +  for (auto& gEntitie : gEntities) {

Same here.

::: xpcom/tests/gtest/TestHashtables.cpp:407
(Diff revision 2)
> -    foo->SetString(nsDependentCString(gEntities[i].mStr));
> +    foo->SetString(nsDependentCString(gEntitie.mStr));
>  
> -    UniToEntClass2.Put(gEntities[i].mUnicode, foo);
> +    UniToEntClass2.Put(gEntitie.mUnicode, foo);
>    }
>  
> -  for (uint32_t i = 0; i < ENTITY_COUNT; ++i) {
> +  for (auto& gEntitie : gEntities) {

Sam here.
Attachment #8811227 - Flags: review?(nfroyd) → review+
Comment on attachment 8811228 [details]
Bug 1317954 - Use auto type specifier where aplicable for variable declarations to improve code readability and maintainability in xpcom/.

https://reviewboard.mozilla.org/r/93420/#review93866

r=me with every instance of `auto *` added fixed as below.

::: xpcom/components/nsCategoryManager.cpp:116
(Diff revision 2)
>  {
>    if (mSimpleCurItem >= mCount) {
>      return NS_ERROR_FAILURE;
>    }
>  
> -  nsSupportsDependentCString* str =
> +  auto *str = new nsSupportsDependentCString(mArray[mSimpleCurItem++]);

We always place the `*` with the type in Gecko, so this would be `auto* str = ...`.  Likewise for every declaration that was changed in this patch.
Attachment #8811228 - Flags: review?(nfroyd) → review+
Comment on attachment 8811229 [details]
Bug 1317954 - Replace default bodies of special member functions with = default; in xpcom/.

https://reviewboard.mozilla.org/r/93422/#review93868

::: xpcom/build/FileLocation.cpp:13
(Diff revision 2)
>  FileLocation::FileLocation()
>  {
>  }

Why does this one not get changed?
Attachment #8811229 - Flags: review?(nfroyd) → review+
Comment on attachment 8811230 [details]
Bug 1317954 - Replace string literals containing escaped characters with raw string literals in xpcom/.

https://reviewboard.mozilla.org/r/93424/#review93870

::: xpcom/tests/TestArguments.cpp:21
(Diff revision 2)
>        return 5;
>    if (strcmp("bar", argv[6]) != 0)
>        return 6;
>    if (strcmp("argument with spaces", argv[7]) != 0)
>        return 7;
> -  if (strcmp("\"argument with quotes\"", argv[8]) != 0)
> +  if (strcmp(R"("argument with quotes")", argv[8]) != 0)

I don't know that the raw string syntax is really any better than the alternative.  I also wonder whether it's going to cause issues for syntax highlighting in editors...
Attachment #8811230 - Flags: review?(nfroyd) → review+
Comment on attachment 8811231 [details]
Bug 1317954 - Use C++11's override and remove virtual where applicable in xpcom/.

https://reviewboard.mozilla.org/r/93426/#review93872

r=me, I guess, though the `override` on the destructor makes little sense.

::: xpcom/build/NSPRInterposer.cpp:39
(Diff revision 2)
>    explicit NSPRIOAutoObservation(IOInterposeObserver::Operation aOp)
>      : IOInterposeObserver::Observation(aOp, "NSPRIOInterposer")
>    {
>    }
>  
> -  ~NSPRIOAutoObservation()
> +  ~NSPRIOAutoObservation() override

Is this really necessary?
Attachment #8811231 - Flags: review?(nfroyd) → review+
(In reply to Nathan Froyd [:froydnj] from comment #14)
> Comment on attachment 8811230 [details]
> Bug 1317954 - Replace string literals containing escaped characters with raw
> string literals in xpcom/.
> 
> https://reviewboard.mozilla.org/r/93424/#review93870
> 
> ::: xpcom/tests/TestArguments.cpp:21
> (Diff revision 2)
> >        return 5;
> >    if (strcmp("bar", argv[6]) != 0)
> >        return 6;
> >    if (strcmp("argument with spaces", argv[7]) != 0)
> >        return 7;
> > -  if (strcmp("\"argument with quotes\"", argv[8]) != 0)
> > +  if (strcmp(R"("argument with quotes")", argv[8]) != 0)
> 
> I don't know that the raw string syntax is really any better than the
> alternative.  I also wonder whether it's going to cause issues for syntax
> highlighting in editors...

I've tried it on 3 editors, visual studio, eclipse and xcode and they all support raw string literals highlight so i guess it's safe.
(In reply to Nathan Froyd [:froydnj] from comment #15)
> Comment on attachment 8811231 [details]
> Bug 1317954 - Use C++11's override and remove virtual where applicable in
> xpcom/.
> 
> https://reviewboard.mozilla.org/r/93426/#review93872
> 
> r=me, I guess, though the `override` on the destructor makes little sense.
> 
> ::: xpcom/build/NSPRInterposer.cpp:39
> (Diff revision 2)
> >    explicit NSPRIOAutoObservation(IOInterposeObserver::Operation aOp)
> >      : IOInterposeObserver::Observation(aOp, "NSPRIOInterposer")
> >    {
> >    }
> >  
> > -  ~NSPRIOAutoObservation()
> > +  ~NSPRIOAutoObservation() override
> 
> Is this really necessary?

You're right but still having the override qualifier is a guarantee that the base destructor is virtual.
Pushed by bpostelnicu@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/350a507039f3
Converts for(...; ...; ...) loops to use the new range-based loops in C++11 in xpcom/. r=froydnj
https://hg.mozilla.org/integration/autoland/rev/0f6bb0d8379b
Use auto type specifier where aplicable for variable declarations to improve code readability and maintainability in xpcom/. r=froydnj
https://hg.mozilla.org/integration/autoland/rev/c270b3d62780
Replace default bodies of special member functions with = default; in xpcom/. r=froydnj
https://hg.mozilla.org/integration/autoland/rev/7011bfbad725
Replace string literals containing escaped characters with raw string literals in xpcom/. r=froydnj
https://hg.mozilla.org/integration/autoland/rev/2b5fe4f35038
Use C++11's override and remove virtual where applicable in xpcom/. r=froydnj
Product: Core → Firefox Build System
Product: Firefox Build System → Developer Infrastructure
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: