How to "set" nonstandard datatypes

[DavidPoliakoff]

Right now cali::Annotation will set an int, double, const char*, or Variant. Are there plans to expand this group? I worry about casting (for example) uint64_t's to doubles all over the place.

[daboehme]

This could be easily added in a PR :)

[DavidPoliakoff]

Any concern over doing this with, say, a lot of types?

[mrzv]

I started filing a separate issue, but GitHub helpfully pointed me here. I would cast another vote for having more overloads of Annotation::set(). It would be nice to at least include everything in Variant, but ideally you'd expand it even further. I wanted to record the size of a vector and the code to do that looks like:

    std::vector<int> v;
    cali::Annotation::Guard g( cali::Annotation("x").set(cali::Variant(static_cast<uint64_t>(v.size()))) );

plus including caliper/common/Variant.h. I feel

    cali::Annotation::Guard g( cali::Annotation("x").set(v.size())) );

would be more readable.

[DavidPoliakoff]

Yeah, is this still true? That Annotation.h must be C++98 compatible? I'd like to do something to make it accept anything from which a Variant can be constructed and just enhance Variant, but that requires 11. If we don't, then any time we want to add Variant overloads, we need to add overloads for everything which uses Variants under the hood, which is code duplication

[daboehme]

Hm - could we #ifdef that?

[DavidPoliakoff]

We certainly could, but we'd have people seeing their mysteriously stop compiling with Caliper when they forget to set -std=c++11 or use a compiler which doesn't support it

[mrzv]

Is it not possible to template<class T> set(const T& x) { return set(Variant(x)); }

and then specialize this for T = const char*? That's still C++98.

[DavidPoliakoff]

You can do that as somewhere in between. Essentially anything which can be constructed from a single argument as a Variant just uses the base templates, with specializations for multi-arg. With 11 that can become a variadic, and then all we're doing is forwarding to Variant. If we can, it would be nice to just use the forwarding

[mrzv]

I guess you could have both with #ifdef: old boring path for C++98, new shiny forwarding for C++11.

While I'm here, it would be nice to include size_t in the variant. It's such a basic staple in C++ that having an extra static_cast<uint64_t> is tedious.

[DavidPoliakoff]

Okay, that seems reasonable, if we ever get a "three weeks ago my code compiled with Caliper, now I have some ugly template error" we should just remember this conversation.

@daboehme : do we still want the old interface accepting the regular types? If not, that would be great news

[daboehme]

Well the #ifdef is precisely there to avoid the "suddenly doesn't compile anymore" problem.

As for the old interface: The annotation class has special sauce for int (allowing you to assign ints to double or uint attributes) that should stick around, otherwise we can defer to the template version.

[DavidPoliakoff]

@daboehme : cool. The ifdef protects us from things not suddenly compiling with the current interface. My worry is that we add more special sauce in Variant, forget it in Annotation, and the template solution covers us with C++11 but not pre-11. That said, you're a much more careful developer than I am, perhaps that won't come to pass.

[jrmadsen]

Just a thought here, maybe just put this in the header

#if  __cplusplus <= 199711L
#    if !defined(CALIPER_WARN_CPP98)
#        warning "Caliper has deprecated pre-C++11 support because it is 2019 and C++11 is the default for many newer compilers (e.g. GCC 8)"
#    endif
#endif 

... although maybe slightly less snarky :)

Then you can set up something like this through ~here

[DavidPoliakoff]

@jrmadsen : we'd want to avoid deprecating that support. We've got customers who strongly want such support so we wouldn't actually deprecate it, and the warning spew wouldn't win us any friends.

[jrmadsen]

Haha yea I figured the warnings spew wouldn't win y'all any friends, which is why I included the slightly misnamed !defined(CALIPER_WARN_CPP98) but wow, I guess I didn't realize y'all still wanted to support C++98... ha I'd bump my project up to C++14 already if I felt there was anywhere close to the difference between 11 and 14 compared to 98 and 11 (but then again, I get to avoid the C++11 ABI issues bc there is no linking requirement).

[DavidPoliakoff]

@jrmadsen : I'm mostly with you on 98, but there are good reasons to support 03 (and honestly, if you write your code to 03, writing it to 98 isn't that hard). Last I checked a lot of the NNSA codes still ran on Sequoia very productively, and some don't use Clang. In addition, some have DOD customers who run on old machines. There are people who will honestly yell at us if we decide user-facing stuff is C++11.

With Caliper's model of being built alongside the codes the C++11 linking issues aren't too ugly, we just want to avoid introducing semantics in user-facing code that tie us to a future standard. Within the core (away from users) we tend to be a lot less restrictive.

[DavidPoliakoff]

@daboehme @mrzv : what overloads have been a pain for us in the past? We have size_t, I'll try to get some others, but it would be nice to knock this all out in one PR

[mrzv]

So far I only have size_t, but I've been using Caliper for only a week. If anything else comes up, I'll let you know, but I'm guessing it won't in the short term.

[daboehme]

Well const char* has always been a major hassle. I frankly should have added that from the start. This should also get a C cali_make_variant_from_string function. I would not add std::string, because then people might think the Variant manages the data, which it doesn't.

[DavidPoliakoff]

@daboehme then #202 , we can add more as we need it

[DavidPoliakoff]

I think it's fair to close this, reopen as needed