Closed Bug 1317241 Opened 3 years ago Closed 3 years ago

Run clang-tidy on dom/ module

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

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

We could also break this into sub bugs if there are two many modified files.
Assignee: nobody → bpostelnicu
Attachment #8810349 - Flags: review?(amarchesini)
Attachment #8810350 - Flags: review?(amarchesini)
Attachment #8810351 - Flags: review?(amarchesini)
Attachment #8810352 - Flags: review?(amarchesini)
Attachment #8810353 - Flags: review?(amarchesini)
Attachment #8810354 - Flags: review?(amarchesini)
Comment on attachment 8810349 [details]
Bug 1317241 - Converts for(...; ...; ...) loops to use the new range-based loops in C++11 in dom/.

https://reviewboard.mozilla.org/r/92696/#review93014

::: dom/asmjscache/AsmJSCache.cpp:1861
(Diff revision 3)
>  
>  bool
>  ParamTraits<Metadata>::Read(const Message* aMsg, PickleIterator* aIter,
>                              paramType* aResult)
>  {
> -  for (unsigned i = 0; i < Metadata::kNumEntries; i++) {
> +  for (auto & entry : aResult->mEntries) {

auto&

::: dom/base/DOMIntersectionObserver.cpp:127
(Diff revision 3)
>    }
>  
>    mRootMargin = value.GetRectValue();
>  
> -  for (uint32_t i = 0; i < ArrayLength(nsCSSRect::sides); ++i) {
> -    nsCSSValue value = mRootMargin.*nsCSSRect::sides[i];
> +  for (auto side : nsCSSRect::sides) {
> +    nsCSSValue value = mRootMargin.*side;

nsCSSValue&

::: dom/base/DOMIntersectionObserver.cpp:127
(Diff revision 3)
>    }
>  
>    mRootMargin = value.GetRectValue();
>  
> -  for (uint32_t i = 0; i < ArrayLength(nsCSSRect::sides); ++i) {
> -    nsCSSValue value = mRootMargin.*nsCSSRect::sides[i];
> +  for (auto side : nsCSSRect::sides) {
> +    nsCSSValue value = mRootMargin.*side;

nsCSSValue&

::: dom/base/nsContentUtils.cpp:7128
(Diff revision 3)
>      "accept-charset", "accept-encoding", "access-control-request-headers",
>      "access-control-request-method", "connection", "content-length",
>      "cookie", "cookie2", "date", "dnt", "expect", "host", "keep-alive",
>      "origin", "referer", "te", "trailer", "transfer-encoding", "upgrade", "via"
>    };
> -  for (uint32_t i = 0; i < ArrayLength(kInvalidHeaders); ++i) {
> +  for (auto & kInvalidHeader : kInvalidHeaders) {

auto&

::: dom/base/nsContentUtils.cpp:9151
(Diff revision 3)
>    };
>    static mozilla::BloomFilter<12, nsIAtom> sFilter;
>    static bool sInitialized = false;
>    if (!sInitialized) {
>      sInitialized = true;
> -    for (uint32_t i = 0; i < ArrayLength(nonEscapingElements); ++i) {
> +    for (auto & nonEscapingElement : nonEscapingElements) {

auto&

::: dom/base/nsContentUtils.cpp:9158
(Diff revision 3)
>      }
>    }
>  
>    nsIAtom* tag = aParent->NodeInfo()->NameAtom();
>    if (sFilter.mightContain(tag)) {
> -    for (uint32_t i = 0; i < ArrayLength(nonEscapingElements); ++i) {
> +    for (auto & nonEscapingElement : nonEscapingElements) {

auto&
Attachment #8810349 - Flags: review?(amarchesini) → review+
Comment on attachment 8810350 [details]
Bug 1317241 - Use auto type specifier where aplicable for variable declarations to improve code readability and maintainability in dom/.

https://reviewboard.mozilla.org/r/92698/#review93018

::: dom/base/nsFrameMessageManager.cpp:1845
(Diff revision 3)
>      uri->GetScheme(scheme);
>      // We don't cache data: scripts!
>      if (aShouldCache && !scheme.EqualsLiteral("data")) {
>        // Root the object also for caching.
> -      nsMessageManagerScriptHolder* holder =
> +      auto* holder =
>          new nsMessageManagerScriptHolder(cx, script, aRunInGlobalScope);

maybe this can stay on the same line

::: dom/base/nsFrameMessageManager.cpp:2207
(Diff revision 3)
>      cb = new SameChildProcessMessageManagerCallback();
>    } else {
>      cb = new ChildProcessMessageManagerCallback();
>      RegisterStrongMemoryReporter(new MessageManagerReporter());
>    }
> -  nsFrameMessageManager* mm = new nsFrameMessageManager(cb,
> +  auto* mm = new nsFrameMessageManager(cb,

indentation!
Attachment #8810350 - Flags: review?(amarchesini) → review+
Comment on attachment 8810351 [details]
Bug 1317241 - Replace default bodies of special member functions with = default in dom/.

https://reviewboard.mozilla.org/r/92700/#review93020

::: dom/indexedDB/ActorsParent.cpp:337
(Diff revision 3)
>    bool
>    HasLiveIndexes() const;
>  
>  private:
>    ~FullObjectStoreMetadata()
> -  { }
> +  = default;

same line.

::: dom/indexedDB/ActorsParent.cpp:369
(Diff revision 3)
>    already_AddRefed<FullDatabaseMetadata>
>    Duplicate() const;
>  
>  private:
>    ~FullDatabaseMetadata()
> -  { }
> +  = default;

same line
Attachment #8810351 - Flags: review?(amarchesini) → review+
Comment on attachment 8810352 [details]
Bug 1317241 - Replace string literals containing escaped characters with raw string literals in dom/.

https://reviewboard.mozilla.org/r/92702/#review93022

I like this so much!
Attachment #8810352 - Flags: review?(amarchesini) → review+
Comment on attachment 8810353 [details]
Bug 1317241 - Replace integer literals which are cast to bool in dom/.

https://reviewboard.mozilla.org/r/92704/#review93024
Attachment #8810353 - Flags: review?(amarchesini) → review+
Comment on attachment 8810354 [details]
Bug 1317241 - Use C++11's override and remove virtual where applicable in dom/

https://reviewboard.mozilla.org/r/92706/#review93026

You must work on this patch a bit more. Please, land the patch only when all the extra spaces, and the indentation issues are fixed.

::: dom/base/nsFrameMessageManager.cpp:1950
(Diff revision 5)
> -  virtual ~SameParentProcessMessageManagerCallback()
> +  ~SameParentProcessMessageManagerCallback() override
>    {
>      MOZ_COUNT_DTOR(SameParentProcessMessageManagerCallback);
>    }
>  
> -  virtual bool DoLoadMessageManagerScript(const nsAString& aURL,
> +  bool DoLoadMessageManagerScript(const nsAString& aURL,

indentation

::: dom/base/nsFrameMessageManager.cpp:2022
(Diff revision 5)
>    {
>      MOZ_COUNT_DTOR(ChildProcessMessageManagerCallback);
>    }
>  
> -  virtual bool DoSendBlockingMessage(JSContext* aCx,
> +  bool DoSendBlockingMessage(JSContext* aCx,
>                                       const nsAString& aMessage,

indentation

::: dom/base/nsFrameMessageManager.cpp:2050
(Diff revision 5)
>      }
>      return cc->SendRpcMessage(PromiseFlatString(aMessage), data, cpows,
>                                IPC::Principal(aPrincipal), aRetVal);
>    }
>  
> -  virtual nsresult DoSendAsyncMessage(JSContext* aCx,
> +  nsresult DoSendAsyncMessage(JSContext* aCx,

indentation

::: dom/base/nsFrameMessageManager.cpp:2110
(Diff revision 5)
> -  virtual ~SameChildProcessMessageManagerCallback()
> +  ~SameChildProcessMessageManagerCallback() override
>    {
>      MOZ_COUNT_DTOR(SameChildProcessMessageManagerCallback);
>    }
>  
> -  virtual bool DoSendBlockingMessage(JSContext* aCx,
> +  bool DoSendBlockingMessage(JSContext* aCx,

indentation

::: dom/base/nsFrameMessageManager.cpp:2130
(Diff revision 5)
>                            true, &aData, &cpows, aPrincipal, aRetVal);
>      }
>      return true;
>    }
>  
> -  virtual nsresult DoSendAsyncMessage(JSContext* aCx,
> +  nsresult DoSendAsyncMessage(JSContext* aCx,

indentation

::: dom/base/nsGlobalWindow.cpp:722
(Diff revision 5)
>  class nsOuterWindowProxy : public js::Wrapper
>  {
>  public:
>    constexpr nsOuterWindowProxy() : js::Wrapper(0) { }
>  
> -  virtual bool finalizeInBackground(const JS::Value& priv) const override {
> +  bool finalizeInBackground(const JS::Value& priv) const override {

{ in a new line

::: dom/base/nsGlobalWindow.cpp:727
(Diff revision 5)
> -  virtual bool finalizeInBackground(const JS::Value& priv) const override {
> +  bool finalizeInBackground(const JS::Value& priv) const override {
>      return false;
>    }
>  
>    // Standard internal methods
> -  virtual bool getOwnPropertyDescriptor(JSContext* cx,
> +  bool getOwnPropertyDescriptor(JSContext* cx,

indentation... here and _everywhere_ in this patch :)

::: dom/base/nsGlobalWindow.cpp:9065
(Diff revision 5)
>  
>  // Try to match compartments that are not web content by matching compartments
>  // with principals that are either the system principal or an expanded principal.
>  // This may not return true for all non-web-content compartments.
>  struct BrowserCompartmentMatcher : public js::CompartmentFilter {
> -  virtual bool match(JSCompartment* c) const override
> +  bool match(JSCompartment* c) const override

in the meantime: c => aC

::: dom/indexedDB/ActorsParent.cpp:5475
(Diff revision 5)
> -  virtual
> -  ~ConnectionRunnable() = default;
> +  
> +  ~ConnectionRunnable() override = default;

remove these extra spaces

::: dom/indexedDB/ActorsParent.cpp:5900
(Diff revision 5)
>      , mActorDestroyed(false)
>    {
>      AssertIsOnOwningThread();
>    }
>  
> -  virtual
> +  

extra spaces

::: dom/indexedDB/ActorsParent.cpp:6106
(Diff revision 5)
>    TransactionDatabaseOperationBase(TransactionBase* aTransaction);
>  
>    TransactionDatabaseOperationBase(TransactionBase* aTransaction,
>                                     uint64_t aLoggingSerialNumber);
>  
> -  virtual
> +  

ditto

::: dom/indexedDB/ActorsParent.cpp:7376
(Diff revision 5)
>    FactoryOp(Factory* aFactory,
>              already_AddRefed<ContentParent> aContentParent,
>              const CommonFactoryRequestParams& aCommonParams,
>              bool aDeleting);
>  
> -  virtual
> +  

ditto

::: dom/indexedDB/ActorsParent.cpp:7771
(Diff revision 5)
>    }
>  
>  protected:
>    DatabaseOp(Database* aDatabase);
>  
> -  virtual
> +  

ditto

::: dom/indexedDB/ActorsParent.cpp:7833
(Diff revision 5)
>  protected:
>    explicit VersionChangeTransactionOp(VersionChangeTransaction* aTransaction)
>      : TransactionDatabaseOperationBase(aTransaction)
>    { }
>  
> -  virtual
> +  

ditto

::: dom/indexedDB/ActorsParent.cpp:8133
(Diff revision 5)
>  #ifdef DEBUG
>      , mResponseSent(false)
>  #endif
>    { }
>  
> -  virtual
> +  

in this patch, there are many!
Attachment #8810354 - Flags: review?(amarchesini) → review+
Pushed by bpostelnicu@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/494c49290b53
Converts for(...; ...; ...) loops to use the new range-based loops in C++11 in dom/. r=baku
https://hg.mozilla.org/integration/autoland/rev/5b3a57978bd4
Use auto type specifier where aplicable for variable declarations to improve code readability and maintainability in dom/. r=baku
https://hg.mozilla.org/integration/autoland/rev/c1c955254a8c
Replace default bodies of special member functions with = default in dom/. r=baku
https://hg.mozilla.org/integration/autoland/rev/de9b1fc4d31c
Replace string literals containing escaped characters with raw string literals in dom/. r=baku
https://hg.mozilla.org/integration/autoland/rev/06b5e559e2a8
Replace integer literals which are cast to bool in dom/. r=baku
https://hg.mozilla.org/integration/autoland/rev/ad9ca936fa4a
Use C++11's override and remove virtual where applicable in dom/ r=baku
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.