Use explicit llvm:: for STLExtras functions or not? (original) (raw)

It seems we have this unwritten convention of prefixing the STLExtras variants of C++ standard library functions with llvm:: even when it’s not needed. For example, a grep for llvm::sort or llvm::find_if returns several hits in files that have using namepace llvm in them. I have been interpreting it as a way to reinforce that these are the LLVM variants.

But this seems to be used inconsistently. For example, llvm::enumerate is used both with the explicit namespace qualifier and without. Should we standardize on one scheme? Since the llvm versions are not conflicting with the standard C++ ones, and we never should have using namespace std in our code (exception being tests) it seems using these functions without any namespace should be ok from both correct compilation and readability POV as there is no possibility of a conflict/confusion with the std ones as the std ones when used should be prefixed with the std:: namespace qualifier.

Does that make sense?

Thanks
Rahul

@jayfoad FYI

I prefer qualified names in general, except if it would sacrefice readability. For example, if by using qualification would break the line, but would otherwise fit nicely, then I’d opt for not qualifying the call/type.

I’d probably prefer qualify them. The overload resolution surprises that can happen when things are ‘iterator-like-enough’ for the SFINAE can be concerning. I recall being bitten by that at least once in the past.

shafik May 2, 2025, 5:26pm 4

I think when naming overlaps w/ std:: names we should use llvm:: it just clarifies and avoids confusion. Clarity is always good.

I think we’re generally pretty inconsistent about use of llvm:: vs not, not just with STLExtras. For example, in Clang we have plenty of instances of llvm::SmallVector and other containers, even though the llvm:: is almost never required. In general, I think we’re pretty unhygienic about namespaces.

I don’t have a strong opinion about what we should prefer, beyond ensuring that any modifications we make to the code base for consistency are balanced against the pain it causes in merge conflicts (esp for downstreams), as we usually do.

jurahul May 2, 2025, 5:31pm 6

Thanks. I agree that we do not want to do a wholesale change in the code to conform to anything that we might agree on here. That can happen organically over time.

nikic May 2, 2025, 7:43pm 7

I think STLExtras should follow the general rule of not using llvm:: qualification inside llvm/. I don’t think there is much benefit to qualifying STLExtras helpers, and it makes the guidelines more complex.

The one exception I can see to this is cases where an llvm:: function actually has an overload that matches an std:: overload. Normally llvm:: functions have different signatures than std:: functions, because they accept ranges instead of iterator pairs. A notable exception is llvm::sort, which can also take two iterators. I think this particular case might be where the “qualify STLExtras” pattern has originated from. I’m not sure if we have any others like it.

MattPD May 2, 2025, 10:02pm 8

A potential concern could be that an unqualified call could drag in the std version via ADL, say, when searching for std::vector<int>::iterator in a std::vector<std::vector<int>::iterator>, as in https://godbolt.org/z/zG1G7vx9f, although that may be contrived? (It would still result in a diagnostic if llvm::find_if (R &&Range, UnaryPredicate P) was the intent anyway.)

Personally I prefer to always explicitly qualify names to avoid any potential ambiguity (and quickly recognize where a type/function is coming from–particularly in a mixed mlir:: and llvm:: codebase), but understandably preferences may vary.

jurahul May 2, 2025, 10:25pm 9

So maybe the rule of thumb is: if it works without the qualifier, it’s OK to not have the llvm:: (and that’s the default case). However, if between the author and PR reviewer(s) there is a consensus that the extra llvm:: will help even if code compiles without it, that’s OK as well. And if the code does not compile, obviously its needed (for example in Clang or MLIR code). And this is not limited to just STLExtras, just that with STLExtras there may be a stronger reason to use llvm::.

With this, all existing code is conformant by definition as well.

Without providing an opinion on the actual question, this states that personal style is fine. IMHO, we want a coding convention to avoid mixing (personal) styles.

jurahul May 2, 2025, 10:57pm 11

I intentionally made it a preference (to not use llvm::) since it seems we have folks in both camps. But yeah, I agree we want to reach an actual convention to avoid personal styles and code review back-and-forth. Let’s wait for this thread to play out and see if we come up with a convention here.

jh7370 May 6, 2025, 8:07am 12

Preferences aren’t coding standards. We should either a) not talk about it at all in the standards (which essentially means by default it’s “up to the developer/reviewer, with a focus on readability”), or b) define clear guidelines with optional exceptions.

I don’t have strong opinions on the topic, personally, but I think a common standard would be useful. My instinct would be to qualify with the llvm:: namespace any function that is an overload of an existing std:: function, or which is intended to be a function that should be considered for standardization. In particular, this should be the same set of functions that don’t follow the normal LLVM naming conventions and instead follow the std naming conventions. The reason I think this is two-fold. 1) It’s easier to find instances of these with a search, which may one day become relevant if needing to do a bulk find-replace as part of migrating to a standard equivalent. 2) It more clearly communicates that these functions are intended to mirror std versions of functions.

jurahul May 6, 2025, 6:36pm 13

Agreed (as I did above). So let me propose that we make that preference a convention, i.e., for code under llvm/ directory, we should not use the llvm:: namespace qualifier for STLExtras functions (or any other symbol that’s defined in the llvm:: namespace for that matter). The rationale can be as follows:

• These function calls will look consistent with the surrounding code that is also not using llvm::.
• There is no possibility of a conflict with std:: versions of these as no code under llvm/ should have using namespace std (see this section of coding standards). Any calls to std:: should therefore be prefixed with std::. Hence, even for a reader, it should be easy to tell if a call is using std or llvm version without having to prefix both versions.
• Prefixing llvm:: for a possible future replacement with a std:: version seems like carrying the llvm:: baggage around for a long time in preparation for a speculative change. I am assuming the possibility of replacing an llvm:: one with an std:: might open up when we upgrade the C++ version used, which does not happen that often? And when that does happen, finding replacement candidates can be done in other ways without greping for llvm::some_func (for example, grep some_func | grep -v "std::"). At the end of the day, we need to get a clean build, so if we drop the llvm version, we will see build failures for the ones we need to replace with std:: versions without having to grep for anything. Also, not everyone who contributes to LLVM is necessarily aware of what extensions the C++ standard may adopt to even know which llvm:: variants are potentially replaceable and hence need to be prefixed with llvm:: (both when using an existing one or adding a new one to STLExtras).
• Having a special carve out rule for STLExtras (of needing llvm::) seems inconsistent with the rest of the coding standard.

Hence, the convention would be to not use the llvm:: qualifier for these.

They can conflict due to ADL, e.g.: Compiler Explorer
(size may not be the best example, but it is just an example.)

To me, there is no big difference between stl_like_func and llvm::stl_like_func as long as the chosen variant is used consistently.

jurahul May 6, 2025, 7:13pm 15

Thanks. I guess that can happen to any llvm:: function (not just STLExtras). Will such a conflict always result in an error, or can the compiler silently choose the wrong one? If it’s always an error, we can tweak the convention to say no llvm:: unless there is a conflict like this one? That is, add llvm:: only if needed to get the code compiled. The other option (to always require llvm:: seems too burdensome). Thoughts?

We have been qualifying STLExtras for as long as I remember. I think we should stick with it regardless of whether it actually avoids an ADL-induced problem or not.

jurahul May 7, 2025, 12:34pm 18

The whole reason I started this thread is because this seems to be inconsistently applied and we see legitimate review comments like this one:

where there is nothing in the coding convention to point to about this. I see the following choices if we want to settle on something:

1. Don’t add llvm:: unless we run into ADL related ambiguity (i.e., add llvm:: only if it’s needed to get the code compiled). This is assuming compilers will faithfully give an error and not silently choose the wrong version.
2. Always use llvm::. Slightly burdensome but maybe ok.
3. If ADL related subtleties with these functions can go uncaught if someone forgets to add llvm:: and results in surprising bugs, maybe these functions should be in a different namespace than llvm so that they have to be explicitly qualified (and we should never using namespace that namespace). Something like stl_extras::sort as opposed to llvm::sort. That will be a disruptive change but can be done in phases. But really only needed if compiler is unable to catch issues.

Can someone clarify if (3) can happen?

My preference for always qualifying STLExtras is to maintain consistency, plus it’s a very simple convention. I’m not sure if (3) can occur in practice, but at the same time we can’t rule it out. Even if it can’t happen now, it may start happening in the future.

I tend to qualify with llvm::, but I don’t have a strong opinion.