doc: Avoid ADL for function calls #24416

1. 
   hebasto commented at 1:29 pm on February 22, 2022: member

   It happened two times recently, when ADL popped up unexpectedly and brought some confusion:

   • https://github.com/bitcoin/bitcoin/pull/24338/files#r805989994

   Any idea why this even compiles?

   • https://www.erisian.com.au/bitcoin-core-dev/log-2022-02-18.html#l-51:

   2022-02-18T03:24:14 <dongcarl> Does anyone know why this compiles? https://github.com/dongcarl/bitcoin/commit/6d3d2caa3751f968f72d19fa84f001472fb26749 2022-02-18T03:24:14 <dongcarl> GetUTXOStatsWithHasher and MakeUTXOHasher are both in the kernel:: namespace and I never added a using declaration on top… 2022-02-18T03:25:53 <sipa> https://en.cppreference.com/w/cpp/language/adl ?

   Let’s document our intention to avoid similar cases in the future.

2. doc: Avoid ADL for function calls 52a797bfe5
3. hebasto added the label Docs on Feb 22, 2022
4. 
   laanwj commented at 5:29 pm on February 22, 2022: member

   Concept ACK. ADL is unexpected (to many people) and somewhat error prone. A bit like non-explicit constructors (but not that bad). I wonder if there is a way this could be enforced.
5. 
   hebasto commented at 12:01 pm on April 14, 2022: member

   Friendly ping @dongcarl @ryanofsky @MarcoFalke
6. MarcoFalke approved
7. 
   laanwj commented at 6:36 pm on April 21, 2022: member

   FWIW I looked into disabling ADL, but did not find any way that is usable for us. There are some complicated recursive namespace tricks that prevent it from being used in specific cases, but no options to disable it it globally. Also I’ve found no linters / checkers that can check for it and reject it.
8. 
   fanquake commented at 7:19 pm on April 21, 2022: member

   Also I’ve found no linters / checkers that can check for it and reject it.

   Potentially something we can do with some future LLVM / clang-tidy tooling. cc @theuni

9. 
   laanwj commented at 11:32 am on June 1, 2022: member

   but no options to disable it it globally

   Also IIRC disabling it wholesale wouldn’t work because IIRC the C++ library itself relies on it in different places.

   Anyhow, ACK 52a797bfe5ced4329f2272be417c35730ec8839f, there is no need to hold merge up on this, documenting it is a step forward.

10. laanwj merged this on Jun 1, 2022
11. laanwj closed this on Jun 1, 2022

    ---

12. hebasto deleted the branch on Jun 1, 2022
13. 
    theuni commented at 3:20 pm on June 1, 2022: member

    Ah, sorry @hebasto, I just saw the ping on this.

    I had a quick go at a plugin to verify that ADL can be detected conceptually with clang-tidy, and thankfully it just works: https://github.com/theuni/bitcoin-tidy/commit/c6ffc5a768a2e62f6d170d827db77bb221ec4f56

    Using the upstream example:

    0$ /home/cory/dev/llvm-project/build-tools-extra/bin/clang-tidy --load=/home/cory/dev/bitcoin-tidy/build/libbitcoin-tidy.so -checks='-*,bitcoin-*' ../example_adl.cc -- -std=c++17
    11 warning generated.
    2/home/cory/dev/bitcoin-tidy/build/../example_adl.cc:10:8: warning: Use of ADL [bitcoin-adl-use]
    3    y(x); // Matches
    4       ^
    

    As @laanwj mentioned, it’s used extensively by the stdlib, so we may not ever be able to automate this cleanly. Presumably filtering out std:: would be a good start, though.

14. sidhujag referenced this in commit f4540ef36c on Jun 1, 2022
15. DrahtBot locked this on Jun 1, 2023

Contributors
hebasto laanwj MarcoFalke fanquake theuni

Labels
Docs