Custom implementation gives incorrect location when used in a default argument (at least in Mac+clang)

[jmmut]

Hi, thanks for the code, it's great to have this working cross-compiler cross-platform.

However, I noticed that the custom implementation you added doesn't give the expected call site location when used like this:
(example code simplified from https://en.cppreference.com/w/cpp/utility/source_location/current)

#include <iostream>
#include "source_location/source_location.hpp"

source_location src_clone(source_location a = source_location::current())
{
    return a;
}

int main()
{
    auto s2 = src_clone();          // location should point here 
    std::cout
        << s2.line() << ' ' << s2.function_name() << '\n';
}

I tried like this in Mac with Clang12:

$ clang++ -std=c++11 test.cpp 
test.cpp:5:64: warning: predefined identifier is only valid inside function [-Wpredefined-identifier-outside-function]
source_location src_clone(source_location a = source_location::current())
                                                               ^
./source_location/source_location.hpp:64:60: note: expanded from macro 'current'
#define current(args...) current( __LINE__ , 0, __FILE__ , __PRETTY_FUNCTION__ )
                                                           ^
1 warning generated.


$ ./a.out 
4 top level

Instead of "4 top level" I would expect "11 main".

I understand it might not be possible to give the call site location without compiler support (I can't think of a way to make it work) but I was wondering if you are aware of this and if your tool is working as you expected, as there is no mention to this in the readme and I imagine this is the main use case for source_location to avoid littering call site user code.

Your tool is still useful despite this. Thanks again.

[flagarde]

Hello,

Thanks for the nice words. Indeed the expected value is not 4 but as you mentioned I don't think it's possible to obtain the righ value in this case. If you find so nice solution I would really enjoy a PR

Thanks

[flagarde]

I push your code in example.

In my computer with clang13 I obtain :

main
11
15 main

with

#include <iostream>
#include <source_location/source_location.hpp>

source_location src_clone(source_location a = source_location::current())
{
    return a;
}

int main()
{
   source_location loc(source_location::current());
   std::cout << loc.file_name() << std::endl;
   std::cout << loc.function_name() << std::endl;
   std::cout << loc.line() << std::endl;
   auto s2 = src_clone();          // location should point here 
   std::cout
        << s2.line() << ' ' << s2.function_name() << '\n';
}

Could you try to run the example code ?

Thx

[jmmut]

That's interesting. What's your OS? I guess you have the <source_location> header, right? So I guess your program is not using the custom implementation.

I get this with Clang13 (on a Mac that doesn't have the <source_location>, so it's using the custom implementation):

test2.cpp
int main()
11
4 top level

if I change line 15 to auto s2 = src_clone(source_location::current()); I do get your output as expected:

test2.cpp
int main()
11
15 int main()

I've been using this method while developing in Mac. It's not great but does the job.

[jmmut]

Actually, I found in the Clang documentation how to make it work with builtins. (I don't know why the standard "<source_location>" is not available, though).

See the PR I just opened: #9

Cheers

[flagarde]

@jmmut I'm on Linux (Archlinux). Infortunately I don't have access to a MacOS so the tests were made just using with Github actions. And I must confess I don't have knowledge on MacOS and even less on Windows.

I did this project to understand a bit more but as you can see it's far from perfect. Thanks a lot for the PR. By the way I think GCC have __builtin_LINE(). I will verify this and maybe the clang and gcc cases will be merged and use the same macro.

[flagarde]

@jmmut I don't know if your code is working with old Clang versions. It would be nice to add a version check in this case and

[jmmut]

the Clang documentation says the builtins are there since Clang9, but I don't know how to do proper version checks.

I'll try to update the PR later using __has_builtin (https://clang.llvm.org/docs/LanguageExtensions.html#has-builtin) which seems to be the recommended way to do that. I have a linux too so maybe I'll try to use the same builtins for gcc, which will be useful for c++17 and lower.

[flagarde]

From the clang doc

Note

Prior to Clang 10, __has_builtin could not be used to detect most builtin pseudo-functions.

__has_builtin should not be used to detect support for a builtin macro; use #ifdef instead.

I'm working on an other package with such kind of goal version of clang can be check like this but it seems Apple have a different version number than standard clang (values of clang_major are different)

#elif defined(__clang__)
  //Should be include before gcc as it defined some macro of GCC and MSVC
  //Note that marketing version numbers should not be used to check for language features, as different vendors use different numbering schemes. Instead, use the Feature Checking Macros.
  #define KHAOS_COMPILER_VERSION KHAOS_SET_VERSION(__clang_major__, __clang_minor__, __clang_patchlevel__)
  #if defined(__apple_build_version__)
    #define KHAOS_COMPILER_AppleClang KHAOS_COMPILER_VERSION
  #endif

so different checks should be done depending if apple_build_version is defined or not

I will try to find what happens with GCC. GCC has the buildins too but I don't know when it started to have them.

[flagarde]

@jmmut I made some changes It seems clang have biltins only starting from 9.0 while gcc have them at 5.0

Before that we have two option :

1. Set line to 0
2. line = LINE but in some cases the result is not good
   Wich one looks better for you ? I'm not really sure what is the best solution. Do you have some advice...

A third solution would be to add a macro to let the user select the behaviour

[jmmut]

@flagarde

I might be wrong but I think __has_builtin is discouraged for builtin macros, and I think builtin functions are a different thing. Also the docs also say they provide __has_builtin, __has_feature and __has_extension "so that we don't need to resort to fragile compiler version checks". I would still do #if __has_builtin(__builtin_LINE), but of course, feel free to do it however you want. I'm happy with the current version and I don't plan to support compilers older than clang9 in my project.

In my opinion, if the compiler doesn't support anything I would still use LINE rather than 0. The location of the callee is not great but I think it's closer than 0, which kind of means the whole file. Knowing the callee location you can search for usages.

[flagarde]

@jmmut
You are right __has_builtin is much better. and can always fallback to the version check if __has_builtin is not found. GCC took a long time to implement this version>=10.

Thx for the feedback. I will use __has_builtin when available.

Yes, it's better to have LINE rather than 0 but this came with the use of the macro current(), what happens if user do :

#include "source_location/source_location.hpp"

void current(std::string i)
{
  std::cout<<i<<std::endl;
}