Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

template parameter "T" is not used in declaring the parameter types of function template #386

Closed
MedicineYeh opened this issue Dec 8, 2016 · 9 comments
Assignees
Labels

Comments

@MedicineYeh
Copy link

Hello,
I tried different compilers with this super great repo. Both GCC and Clang can compile it without any warning and error. However, due to some unknown issue, Intel ICC seems to implement this template declaration check differently.
Here is the warning message I got when I compile with the icpc (ICC) 16.0.3 20160415.

In file included from ./json.cc(1):
./json.hpp(1335): warning #488: template parameter "T" is not used in declaring the parameter types of function template "nlohmann::basic_json<ObjectType, ArrayType, StringType, BooleanType, NumberIntegerType, NumberUnsignedType, NumberFloatType, AllocatorType>::basic_json<T,<unnamed>>(NumberIntegerType)"
      template<typename T, typename std::enable_if<
                        ^

In file included from ./json.cc(1):
./json.hpp(1430): warning #488: template parameter "T" is not used in declaring the parameter types of function template "nlohmann::basic_json<ObjectType, ArrayType, StringType, BooleanType, NumberIntegerType, NumberUnsignedType, NumberFloatType, AllocatorType>::basic_json<T,<unnamed>>(NumberUnsignedType)"
      template<typename T, typename std::enable_if<

I found a way to walk around it, which is using the template type instead of using the specific type on line 1338 and 1433.
Here is my walk around (I only change number_integer_t to T):

 1335     template<typename T, typename std::enable_if<
 1336                  not (std::is_same<T, int>::value) and
 1337                  std::is_same<T, number_integer_t>::value, int>::type = 0>
 1338     basic_json(const T val) noexcept
 1339         : m_type(value_t::number_integer), m_value(val)
 1340     {
 1341         assert_invariant();
 1342     }

Line 1433 is the same thing.

@nlohmann
Copy link
Owner

nlohmann commented Dec 8, 2016

Hey @MedicineYeh, thanks for the report. I don't exactly remember why I had to add T in this indirect style, but there is a comment:

    @tparam T A helper type to remove this function via SFINAE in case @ref
    number_integer_t is the same as `int`. In this case, this constructor
    would have the same signature as @ref basic_json(const int value). Note
    the helper type @a T is not visible in this constructor's interface.

It may be possible to change the code, but I think that this may break the code on MSVC or any other compiler which made me code it this way in the first place.

@nlohmann nlohmann added kind: enhancement/improvement state: please discuss please discuss the issue or vote for your favorite option labels Dec 8, 2016
@gregmarr
Copy link
Contributor

gregmarr commented Dec 8, 2016

@nlohmann I think changing number_integer_t to T in the function parameter list should be a safe change.

@nlohmann nlohmann self-assigned this Dec 9, 2016
@nlohmann nlohmann added this to the Release 2.0.9 milestone Dec 9, 2016
@MedicineYeh
Copy link
Author

Hi @nlohmann @gregmarr ,
Thanks for your replies. I think using T should be safe to any compiler since it's basically the same thing of written number_integer since you have a type checking on the constructor. It depends on whether you want to make this function a real template function or a function with template type check. I usually prefer the later since it's more clear on types. However, the chain constructor tells the types already, so it doesn't really matter to me.
I don't have a MSVC compiler. It's worthy to try if there's any problem. Thanks.

@nlohmann
Copy link
Owner

nlohmann commented Dec 9, 2016

When I make this change

diff --git a/src/json.hpp b/src/json.hpp
index 6fed0a12..c0535cea 100644
--- a/src/json.hpp
+++ b/src/json.hpp
@@ -1335,7 +1335,7 @@ class basic_json
     template<typename T, typename std::enable_if<
                  not (std::is_same<T, int>::value) and
                  std::is_same<T, number_integer_t>::value, int>::type = 0>
-    basic_json(const number_integer_t val) noexcept
+    basic_json(const T val) noexcept
         : m_type(value_t::number_integer), m_value(val)
     {
         assert_invariant();

I get this compiler error:

src/unit-constructor1.cpp:368:18: error: call to constructor of 'json' (aka 'basic_json<>') is ambiguous
            json j(n);
                 ^ ~
../src/json.hpp:1338:5: note: candidate constructor [with T = long long, $1 = 0]
    basic_json(const T val) noexcept
    ^
../src/json.hpp:1406:5: note: candidate constructor [with CompatibleNumberIntegerType = long long, $1 = 0]
    basic_json(const CompatibleNumberIntegerType val) noexcept
    ^
../src/json.hpp:1306:5: note: candidate constructor
    basic_json(boolean_t val) noexcept
    ^
../src/json.hpp:1369:5: note: candidate constructor
    basic_json(const int val) noexcept
    ^
../src/json.hpp:1495:5: note: candidate constructor
    basic_json(const number_float_t val) noexcept
    ^
../src/json.hpp:1961:5: note: candidate constructor
    basic_json(const basic_json& other)
    ^
../src/json.hpp:2038:5: note: candidate constructor
    basic_json(basic_json&& other) noexcept
    ^

Tried with Apple LLVM version 8.0.0 (clang-800.0.42.1) and g++-6 (Homebrew gcc 6.2.0) 6.2.0.

@MedicineYeh
Copy link
Author

Cool, so this work around do make some ambiguity... Seems like my work around is not a good way to fix the warning. Still, the warning is not harmful so I would like to conclude with don't change the type. I have no better idea on fixing this warning. I would consider to send a report to Intel so that they might fix this in the future version. It's really non sense of reporting warning on this peice of code.
By the way, my Linux compiler versions are (GCC) 6.2.1 20160830 and clang version 3.9.0.

@nlohmann nlohmann removed this from the Release 2.0.9 milestone Dec 10, 2016
@nlohmann
Copy link
Owner

I think we can close this issue, right?

@nlohmann nlohmann removed the state: please discuss please discuss the issue or vote for your favorite option label Dec 12, 2016
@TurpentineDistillery
Copy link

To follow-up,
ICC has #pragma warning disable facilities, so this can be silenced in compiler-specific manner.

@MedicineYeh
Copy link
Author

Cool. Thanks everyone.

@owlas
Copy link

owlas commented Jan 26, 2017

@TurpentineDistillery also possible on the command line with -wd488 icc docs

@nlohmann nlohmann added the platform: icc related to Intel compiler label Sep 29, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

5 participants