google / benchmark

A microbenchmark support library
Apache License 2.0
8.93k stars 1.61k forks source link

[FR] BENCHMARK_VERSION is not set by default #1768

Closed PhilipDeegan closed 6 months ago

PhilipDeegan commented 6 months ago

Is your feature request related to a problem? Please describe. Since a recent addition adding usage of "BENCHMARK_VERSION", this project is now coupled to the official build tools and is not compatible with anything else.

Describe the solution you'd like Default macro variables so we can ignore if they're not set

Describe alternatives you've considered Setting BENCHMARK_VERSION during compilation via -DBENCHMARK_VERSION=version doesn't work as the macro is not converted to a string, see below.

Additional context In my experience, converting define inputs to strings is better done via a helper macro like

#define STRINGIFY(x) #x
#define TOSTRING(x) STRINGIFY(x)

such that inputs like -DBENCHMARK_VERSION=version

can be used like

std::string s{TOSTRING(BENCHMARK_VERSION)};

This IMO is simpler from a build perspective to not need any fancy escaping

see: https://coliru.stacked-crooked.com/a/ae9e4273382fe470

LebedevRI commented 6 months ago

I'm not sure i follow. I think there's some deeper context missing here. Sure, we could sink wrapping BENCHMARK_VERSION into "'s (i.e. stringification) down into the source code, but why?

PhilipDeegan commented 6 months ago

but why?

to make it easier for anyone not using cmake or bazel to not care about the macro not being set

Either that or default the macro if it's not set, either or tbh

could be as simple as

#ifndef BENCHMARK_VERSION
#define BENCHMARK_VERSION \"main\" // this is probably badly escaped 
#endif
dmah42 commented 6 months ago

if you're not using cmake or bazel, i think there's probably other issues that you're going to be working around.

but patches welcome, and doing this would probably clean up the awful BUILD.bazel escaping i had to do to make it work there.

PhilipDeegan commented 6 months ago

i think there's probably other issues that you're going to be working around.

Everything was just fine for me until I updated to HEAD ;)

but patches welcome

testing something on my fork atm