In D75041#2799023, @martong wrote:

Perhaps all conversion related logic should go into their own implementation file? Seems like it adds up to roughly 1000 lines.

What about this patch? I'm removing my reviewer bit just so it doesn't appear in my list anymore, but if there are any updates, I'll keep myself as a subscriber. 🙂

There was a bit of talk on Twitter recently about strong typing which reminded me of this checker! 😁 I'll fix all issues in one go once we're through with all the comments, I'd like to see the full picture of what needs to be done.

Bump. 🙂 This is the last part that's still unreviewed and also the most complex.

In D69560#2747092, @alexfh wrote:

BTW, any ideas why "patch application failed"? (See the Build status in the Diff Detail section)

It would be really nice to get pre-merge checks to work for the patch.

NFC Fix the header guard name lint again, hopefully to the proper value this time. I forgot the CHECK out of it...

• NFC Fix lint of header guard nomenclature
• NFC Tear out obsolete LLVM_DEBUG calls from the implementation

@aaron.ballman @alexfh Bump! How shall we move further?

Adding a few subscribers I could find from the original bugzilla entry, + perhaps a few reviewers to help us.

Rebase over D98635. Highlighting only the parameter's name, not the entire range of the parameter.

• Fixed a few nits.
• Rebased, and added highlighting (under-squiggly) the parameter/return type of conversion operators for extra emphasis.

Made sure that the parameter's name is highlighted promptly in the "first|last parameter of the range is..." note.

[NFC] Rebase.

[NFC] Rebase.

Rebased over D98635, allowing us to properly highlight the found "mixable adjacent parameter range" (assuming it's on the same line, of course):

Ping!

In D98477#2638731, @scott.linder wrote:

I'm glad there is interest from others in the new type, I was worried at first it may be too niche of a use.

I'm not qualified with template magic and I am definitely not a person of authority on this library, so all my comments come from a generally strictly "user's perspective". I would love to have a variant type (it's surprising there never has been one in the code for so long), and I am happy I found this patch instead of trying to get into writing a similar thing myself...

NFC Turned some Optional<T> into just T.

• NFC Fixed some nits
• Added a new check option, MinimumIdentifierNameLength instead of a hardcoded 3 value. Defaults to 3.
• Fixed an issue with heuristics matching only in one direction but not in the other direction silenced a warning that clearly should have been there.

NFC Made the code more legible, updated and clarified some comments, fixed grammar issues.

• Made a precondition check explicit, and added an early return to not evaluate a case where it would return false anyways...

• Added some test cases to ensure edge cases won't crash.

• Made the diagnostic message about "binding power" easier to understand.

• Add const reverse iterator to the default-ignored typename list.

[NFC] Fixed nits and code formatting.

@aaron.ballman Could you please help me understand this pre-merge buildbot? It says that Debian failed, with the following message:

In D69560#2629555, @aaron.ballman wrote:

[...] so please hold off on landing it for a bit in case any of the other reviewers want to weigh in.

Fixed a test file that I originally missed to align with the changes.

In D98635#2626464, @whisperity wrote:

Strange because I specifically ran both check-clang and check-clang-tools locally, but will look into this.

In D98635#2626455, @aaron.ballman wrote:

It looks like there are some unit test failures from the change: https://buildkite.com/llvm-project/premerge-checks/builds/29754#a9b04ab5-e326-4ff1-beea-773f21a33349

NFC I originally wrote a Release Notes entry about this, which I forgot to commit before I generated the patch file I uploaded, this is fixed now.

• Massively refactored and modernised the implementation
• Removed spaghetti code related to the check options and made their storage and defaults clearer
• Fixed modelling issues that caused false positives around lambdas, overloaded call operators, recursive calls, enumconstant arguments
• Made the abbreviation dictionary for the Abbreviations heuristic a check-option
• Made the check message much more legible
• Fixed a severe modelling issue about not respecting or handling record types passed by copy (CXXConstructExpr)
• Wrote the documentation for real, including all heuristics, default values, options, etc. The original documentation was basically empty. For the docs, I used @varjujan's thesis (which subject was this check), which is unfortunately only available in Hungarian, but it helped me understand what some of the enums and the Bound doing on the inside.
• Thanks to having written a proper documentation, I also renamed a few symbols and check options to better tell what they are about.

@aaron.ballman I've reworked and uploaded the final addition to this line, the crown jewel, the very reason this checker was developed (D75041). So, AFAIK, the "let's rewrite the whole thing with better code quality" work can be considered done, and we may move on with the review.

After reading through the diff as rendered by Phabricateur, I realised I left in a few crappy debug prints and commented-out values that are not needed in the end. This is fixed now.

• Rebased over updated, rewritten, newer base patch version, vastly increased the quality of the code.
• Tidied the tests and made them more self-explanatory.
• Implemented handling function pointer conversion (implicit losing of noexcept). This was missing from the previous uploaded patch.
• Ensured that implicit conversion sequences are diagnosed properly, even when typedefs are involved. This means that if the conversion takes 4-5-6 logical steps (e.g. instead of SomeFancyIntTypedef -> DoubleBoxingType, emit SomeFancyIntTypedef -> int -> double -> const double -> DoubleBoxingType (user defined ctor)) it's all explained properly.
• Respect the user's decision about the mixability of T and const T (the previous patch D96355) even in the context of implicit conversions (i.e. if the user did not allow that, do not consider a user-defined type which constructor takes a const mixable with a non-const other parameter).
• Put a diagnostic note to the location of the FuntionDecl of the conversion method involved in the mixability.

• [NFC] Reformat the documentation so option values are in single backticks.

• [NFC] Reformat the documentation so option values are in single backticks.

• [NFC] Reformat the documentation so option values are in single backticks.

• [NFC] Reformat the documentation so option values are in single backticks.

• [NFC] Reformat the documentation so option values are in single backticks.

In D97297#2583391, @Eugene.Zelenko wrote:

Please mention new option in Release Notes.

• [NFC] Realised I forgot to add the new option to the check's documentation. This is fixed now.

• Fixed an issue of the "same function" heuristic that blew up if functions are forward declared.
• [NFC] Optimised "relatedness" is to be checked first (as it's a trivial double hashmap lookup) before the recursive descent of type checking.

• Rebased over updated, rewritten, newer base patch version.
• Added the heuristic for "same member access" to the patch (which was so far only on my development branch).

In D96607#2561287, @njames93 wrote:

I don't see the use case you suggest as a strong enough motivation for this, clang-format does this job very well and we shouldn't be reinventing the wheel here.

Make sure that overloadable binary operators are ignored properly.

• NFC Code style, formatting, wording, etc. changes as per review comments.
• NFC Removed a stale FIXME

NFC (With all the restructuring and trying to figure out how to implement the latest change, I managed to upload the wrong, unclean diff...)

Made detection of macros as "parameter type names" more robust.

• Added a C test file with some C-specific language constructs that we may or may not match.
• Explained limitations with regards to variadics, templates, etc. better in the docs, and added test cases for these too.
• When comparing the type names against the ignored type name list, do not consider PrintingPolicy, but take exactly what's in the source code. This mostly concerns bool and _Bool at face value, so this is tested in the C file too.