Associated run: https://treeherder.mozilla.org/jobs?repo=try&revision=1fee1bd685eceb8c41bf24cdab2be32f5dd6c050&selectedTaskRun=KMwGtbUbS0i0mnHyCPBbKQ.0
I've checked manually the configuration of Linux and Windows build and ENABLE_CLANG_PLUGIN is only passed to debug builds

A few minor nits, a long term suggestion, but LGTM otherwise.

@nika : just merged it as a safety guard but I will remove it in an updated version of https://phabricator.services.mozilla.com/D331993

Thanks for the extra round. Do you need me to commit that patch on your behalf?

This is quite a big allowlist! Is there any migration plan? Not blocking, just me being curious.

LGTM with comment adressed and assuming this compiles fine.

Hey, thanks for looking at this! There a re a few customization points in https://clang.llvm.org/extra/clang-tidy/checks/modernize/use-emplace.html , and it's quite common for clang-tidy passes to be customizable. I'd rather go the "upstream first" approach and extend upstream pass (I can serve you as a reviewer there) instead of maintaingour own flavor.

Good. Your patch looks a lot more like a rewrite of the linter :-) But it also adds more testing. Any reason why you restructured it the way you did? Maybe make it a rewrite patch of this one?

A few minor comments to make it easier to read, but LGTM otherwise

Thanks for your answers. LGTM as is.

In D331993#11555584, @nika wrote:

I've double-checked the spec WRT these specializations, and I'm actually not sure that we're technically allowed to specialize std::default_delete. Specifically, it says:

https://eel.is/c++draft/namespace.std#2
2: Unless explicitly prohibited, a program may add a template specialization for any standard library class template to namespace std provided that
(2.1) the added declaration depends on at least one program-defined type, and
(2.2) the specialization meets the standard library requirements for the original template.

2.1 is satisfied which should allow defining this helper, but I think 2.2 is technically not satisfied as the standard library requirements for std::default_delete state:

https://eel.is/c++draft/unique.ptr.dltr.dflt
constexpr void operator()(T* ptr) const;

• Mandates: T is a complete type.
• Effects: Calls delete on ptr.

None of the specializations we have added call delete on the pointer argument, which I believe means that this is technically UB, and a compliant STL implementation would be allowed to detect that std::default_delete is being used, and invoke delete directly instead of using operator() when destroying a std::unique_ptr.

---

I think I'd prefer it if we changed these callers using custom default_delete implementations to specify explicit deleters instead, though I realize that there are a few other callers which already directly specialize std::default_delete (https://searchfox.org/firefox-main/search?q=struct+default_delete&path=&case=false&regexp=false) for rust FFI. I expect it's unlikely that we're going to run into UB problems in practice with this, but it's generally better to follow the rules precisely.

Looks good. I wrote a linter extension that detected those cases in https://phabricator.services.mozilla.com/D332807 as a follow-up

Alternatively, you could move the definition that require nsString.h instead of nsStringFwd to toolkit/mozapps/defaultagent/SetDefaultBrowser.cpp. This keeps the header lean.

constexpr and inline are redundant (pick constexpr ;-))

Thanks!

This revision requires a Testing Policy Project Tag to be set before landing. Please apply one of testing-approved, testing-exception-unchanged, testing-exception-ui, testing-exception-elsewhere, testing-exception-other. Tip: this Firefox add-on makes it easy!