In D59999#1494991, @mclow.lists wrote:

I'm not happy with this. In particular, it does NOT call through the character traits, which it is required to do by the standard.

As an example of code reduction.

Another option for the optimzation.

In D59999#1453709, @mclow.lists wrote:

In D59999#1448376, @sbenza wrote:

Thanks for debugging that. I didn't realize it didn't try to inline on purpose.
Maybe we just need to mark the function as inline, even if we also have the extern template instantiation.

I don't think so, because then it would (might?) not end up in the dylib, and that would break *everything* that uses it.

Use inline to solve the problem instead of changing the implemetation.

Thanks for debugging that. I didn't realize it didn't try to inline on purpose.
Maybe we just need to mark the function as inline, even if we also have the extern template instantiation.
That way the compiler can inline it if it helps, but won't actually generate the weak symbol in the object files otherwise.
Eg, something like:

Maybe the generated code would be better if basic_string::compare was being inlined, but the compiler chose not to do it. char_traits::compare, on the other hand is much smaller and easier to inline.

Remove string file changes from this PR.
They were added by mistake.

• Make it easier for the compiler to optimize operator==(string,char*).

Fix benchmark for rhs being empty.

In D18914#396149, @mgehre wrote:

...
I personally think that 1) should be used, because late one could move the function definition to a source file (removing the inline) without having to touch
the class declaration. I can extend this patch to transform 2) and 3) into 1).

Alternatively, I could add an option to choose between 1), 2) or 3).
What do you think?

In D53994#1319418, @dyaroshev wrote:

In D53994#1319307, @sbenza wrote:

re resize+iota, I don't see why not. That code is not part of the measurements.

As for why 0...n, for integers it doesn't really what the values are. only that there are N distinct values. The initial ordering of those values is changed depending on the benchmark.
Note that for strings we don't use consecutive values as the value itself matters for operator<. That is, the first mismatched char is the one that matters and "sequential" strings would not be good for that.

For lower_bound you might want a different sequence, though. You might also want to check for values missing in the list. The way we do it for std::set is to have 0,2,...,N. We then search for 2*i for hits and 2*i+1 for misses.

I see your point about n values but I'm not entirely sold.
There is a famous bug in quick sort implementation, for example, - not partitioning the duplicates.
BTW just ran a small benchmark, check out how much duplicates affect the performance of std::sort http://quick-bench.com/mq8UiBPBT5vmmS7FG-RmY11VriQ

It seems like the input/benchmark generation in algorithms.bench is not a good fit for binary search.

Also, please take a look: https://github.com/llvm-mirror/libcxx/blob/master/benchmarks/algorithms.bench.cpp#L45
I'm not sure if it matters, but seems like a weird idea to measure on 0...n - not very representative data.
I would also suggest rewriting it like:

V.resize(N);
std::iota(V.begin(), V.end());

The last time I measured - much faster (and less code).

@sbenza wrote those benchmarks, there might be a reason why he used 0...n.

Changed the sizes to be denser in the lower end.

In D54404#1296224, @aaron.ballman wrote:

In D54404#1295426, @steveire wrote:

I acknowledge and share the future-proofing concern.

We could possibly use something trait-based instead and put the trait beside the matcher definition in ASTMatchers.h, but that doesn't really solve the problem. It only moves the problem.

If we could somehow incorporate it into the matcher definition itself, though, it means we don't have two lists of things to keep updated. You're right that it doesn't eliminate the problem -- we still have to know to ask the question when reviewing new matchers (so perhaps something that requires you to explicitly opt-in/out would be better).

Adding @sbenza in case he has ideas.

In D54407#1294934, @aaron.ballman wrote:

Adding @sbenza in case he has opinions on this approach. I think it's reasonable, but I also know that changes to the the AST matcher internals sometimes have unintended side effects with the various instantiations.

In D53978#1283922, @mclow.lists wrote:

Is there any value in breaking these out into one benchmark per file? I can see this file ever-growing as we add new benchmarks.

Sorry for the break.
I submitted the fix.

#include changes. C++17 cleanup.

Fixes

Use TEST_ALWAYS_INLINE instead of attribute((always_inline))

Minor comment fix

Fix casing for some function names.

minor fix

Do we want this change in the parser itself? or in clang-query?
clang-query could be helpful enough to add parens when it detect an ET_ParserNoCloseParen error, without changing the language here.

Missing the .rst file.
Did you use the check_clang_tidy.py script to create the template?