HazardyKnusperkeks wrote:

> > While it is less code, I find a bit harder to understand and the code gen 
> > is far worse: https://gcc.godbolt.org/z/KzG4YnTh3
> 
> `IsBlank` is misleading because of `std::isblank` whereas `Blanks.contains` 
> is not, so the latter has better readability for me. As to the generated 
> code, the latter is worse in runtime (IMO negligible in practice), but the 
> code size is about the same with `-O2` and far better with `-Os` (again, 
> negligible in practice). See [this](https://gcc.godbolt.org/z/YsaKa4hP7) 
> modified example, which adds a call to the local function `IsBlank` to make 
> the comparison fairer.
> 
> More importantly, we wouldn't need to update `IsBlank` if `Blanks` changes. 
> This happened at least once 
> ([580da27](https://github.com/llvm/llvm-project/commit/580da276161ee8b2aa7bc6bd4631cd8261fb9bfb))
>  before.

To make it fair you have to add the `static` to `IsBlank` and then it gets 
inlined again.

It's true, that `Blanks` and `IsBlank` have to be maintained simultaneously, 
but I don't think there will be new _blank_ characters.

And for the readability we obviously disagree.

https://github.com/llvm/llvm-project/pull/146245
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to