Is there a GCC warning for using symbols from the C library not through namespace std?
Asked Answered
A

3

15

Consider the following (buggy) C++ code:

#include <cmath>
#include <cstdlib>

#include <iostream>

int main() {
    if (abs(-0.75) != 0.75) {
        std::cout << "Math is broken!\n";
        return 1;
    } else {
        return 0;
    }
}

This code is buggy because it calls abs (meaning ::abs) instead of std::abs. Depending on the implementation, ::abs might not exist, or it might be the C abs, or it might be an overload set including a version for double, like std::abs is.

With Clang on Linux, at least in my environment, it turns out to be the second option: C abs. This provokes two warnings, even without explicitly enabling any:

<source>:7:9: warning: using integer absolute value function 'abs' when argument is of floating point type [-Wabsolute-value]
    if (abs(-0.75) != 0.75) {
        ^
<source>:7:9: note: use function 'std::abs' instead
    if (abs(-0.75) != 0.75) {
        ^~~
        std::abs
<source>:7:13: warning: implicit conversion from 'double' to 'int' changes value from -0.75 to 0 [-Wliteral-conversion]
    if (abs(-0.75) != 0.75) {
        ~~~ ^~~~~

On GCC, I get different results in different environments and I haven’t yet figured out what details of the environment are relevant. The more common option, though, is also that it calls the C abs function. However, even with -Wall -Wextra -pedantic, it gives no warnings. I can force a warning with -Wfloat-conversion, but that gives too many false positives on the rest of my codebase (which perhaps I should fix, but that’s a different issue):

<source>: In function 'int main()':
<source>:7:18: warning: conversion to 'int' alters 'double' constant value [-Wfloat-conversion]
     if (abs(-0.75) != 0.75) {
                  ^

Is there a way to get a warning whenever I use a library function through the global namespace, when the version in namespace std is an overload?

Ardy answered 13/7, 2017 at 17:55 Comment(12)
Don't spam tags. C does not have a std name-space.Ivories
@Olaf You’re right. It was only relevant because C++ includes large portions of the C standard library, but not in a way that makes C expertise useful for answering this question. I’m sorry.Ardy
This seems to be the same question, but I guess it's answer is not satisfactory either: #9066976Palladous
Using C functions from C++ code never justifies the C tag, standard library or any other library. Otherwise every program would require the tag, as even PHP or Python use those functions.Ivories
@Olaf What I meant was that if, in the process of getting a MCVE, the resulting code was valid as C code (and exhibits the same issue) even if the original code was C++, then I think it would be appropriate to use the C tag. If, as is the case with my question, the MCVE depends on C++ features, then the tag is inappropriate.Ardy
@Palladous Yeah, that is similar; maybe mine should be closed as a duplicate. I didn’t find that when searching.Ardy
I don’t know whether it’s appropriate to flag my question as a duplicate or not; the other one has an accepted answer and no attention for over five years, but the accepted answer doesn’t actually answer the question which was asked (which is what I’m interested in). EDIT: Looking on meta, it looks like the question with a better collection of answers should be kept, and I think that’s already mine, so I’m not flagging as duplicate.Ardy
@DanielH: Identical syntax does not imply identical semantics. Try static const int i = 10, a[i] = {1};.Ivories
@Olaf Yeah, I edited my comment to mention that in a parenthetical. Another example I saw recently is that while (true); is actually UB in C++ ([intro.progress]), but not C; if I were seeing an issue with that the [C] tag wouldn’t be appropriate.Ardy
@DanielH: Differences are quite subtle and the languages are drifting away from each other since C++11 even more. There are more in the future. Just tag with the language you use, don't double-tag (unless it really covers both, i.e. not just the ABI like `extern "C" implies).Ivories
@DanielH: I'll leave the decision to you; I didn't want to wield the hammer in this case. Personally, I'd suggest you fix your code so that you can use -Wfloat-conversion; unintentional float-to-int conversions are bugs-in-waiting and intentional ones can easily be marked as such. But I understand that can be a lot of work.Palladous
@Palladous Yeah, I’d prefer that too; I found this issue because some tests were passing when they shouldn’t be, and probably had been for a while. With the help of sort -u, I’m finding that we actually don’t have as many places where that warning goes off as I thought; I might just fix them. I can’t think of any C library functions which might cause confusion after that, but of course it’s the ones I can’t think of that cause the most trouble.Ardy
L
3

Here's a solution. I'm not happy with it, but it might work for you:

namespace DontUseGlobalNameSpace {
// put all std functions here you want to catch
int abs(int x);
}
using namespace DontUseGlobalNameSpace;

Now, if you use abs() without qualification, you'll get a "symbol is ambiguous" error.

Lamprey answered 13/7, 2017 at 18:59 Comment(5)
That would work, but it requires enumerating all the methods at issue. It wouldn’t take more than an hour to go through cppreference or the standard and look for those, but it’s still not ideal and there’s the possibility of missing something.Ardy
It would also require including dontuseglobalnamespace.hpp at the top of all the source files, which I could accomplish with sed or something but would certainly not be an ideal solution. Especially on an ongoing basis; it’s easier to fix all the issues now than to ensure that they remain fixed as code is updated.Ardy
Yep, that's why I not happy with this solution :) Compilers usually have an option to include a file. For example, gcc has -include to include a file as if you included it at the beginning of the file.Lamprey
Oh, nice, I didn’t know that. That might be enough to let me use this solution, although for now I’ll wait to see if somebody else has a better answer.Ardy
Well, since nobody has had a better idea over the weekend I’m probably going to try this one. If it ends up working out in practice, I’ll accept it.Ardy
I
2

This is going to be difficult. The GCC <cmath> header simply includes <math.h>, #undefs its macros (just in case) and defines the C++ functions as inline functions which make some use of identifiers from <math.h>. Most of the functions in fact refer to compiler builtins: for instance, std::abs is defined using __builtin_abs and not ::abs.

Since <cmath> and your "buggy program" are all in the same translation unit, it's hard to see how the visibility could be separated: how the inline functions in <cmath> could be allowed to use <math.h> stuff, while your code wouldn't.

Well, there is the following way: <cmath> would have to be rewritten to provide its own locally scoped declarations for anything that it needs from <math.h> and not actually include that header.

What we can do instead is prepare a header file which re-declares the functions we don't want, with an __attribute__ ((deprecated)):

// put the following and lots of others like it in a header:
extern "C" int abs(int) throw () __attribute__ ((deprecated));
#include <cmath>
#include <cstdlib>

#include <iostream>

int main() {
  if (abs(-0.75) != 0.75) {
    std::cout << "Math is broken!\n";
    return 1;
  } else {
    return 0;
  }
}

Now:

$ g++ -Wall  buggy.cc
buggy.cc: In function ‘int main()’:
buggy.cc:9:7: warning: ‘int abs(int)’ is deprecated [-Wdeprecated-declarations]
   if (abs(-0.75) != 0.75) {
       ^~~
In file included from /usr/include/c++/6/cstdlib:75:0,
                 from buggy.cc:4:
/usr/include/stdlib.h:735:12: note: declared here
 extern int abs (int __x) __THROW __attribute__ ((__const__)) __wur;
            ^~~
buggy.cc:9:16: warning: ‘int abs(int)’ is deprecated [-Wdeprecated-declarations]
   if (abs(-0.75) != 0.75) {
                ^
In file included from /usr/include/c++/6/cstdlib:75:0,
                 from buggy.cc:4:
/usr/include/stdlib.h:735:12: note: declared here
 extern int abs (int __x) __THROW __attribute__ ((__const__)) __wur;
            ^~~

A linker warning would be simpler. I tried that; the problem is that this test program doesn't actually generate an external reference to abs (even though there is an #undef abs in <cmath>). The call is being inlined, and so evades the linker warning.

Update:

Following up DanielH's comment, I have come up with a refinement of the trick which allows std::abs but blocks abs:

#include <cmath>
#include <cstdlib>
#include <iostream>

namespace proj {
  // shadowing declaration
  int abs(int) __attribute__ ((deprecated));

  int fun() {
    if (abs(-0.75) != 0.75) {
      std::cout << "Math is broken!\n";
      return 1;
    } else {
      return std::abs(-1); // must be allowed
    }
  }
}

int main() {
  return proj::fun();
}

Simple namespaces can be used. Also, we don't need the deprecated attribute; we can just declare abs as an incompatible function, or a non-function identifier entirely:

#include <cmath>
#include <cstdlib>
#include <iostream>

namespace proj {
  // shadowing declaration
  class abs;

  int fun() {
    if (abs(-0.75) != 0.75) {
      std::cout << "Math is broken!\n";
      return 1;
    } else {
      return std::abs(-1); // must be allowed
    }
  }
}

int main() {
  return proj::fun();
}

$ g++ -std=c++98 -Wall  buggy.cc -o buggy
buggy.cc: In function ‘int proj::fun()’:
buggy.cc:10:18: error: invalid use of incomplete type ‘class proj::abs’
     if (abs(-0.75) != 0.75) {
                  ^
buggy.cc:7:9: note: forward declaration of ‘class proj::abs’
   class abs;
         ^~~
buggy.cc:16:3: warning: control reaches end of non-void function [-Wreturn-type]
   }
   ^

With this approach, we just need a list of names and dump them into some header that provides this:

int abs, fabs, ...; // shadow all of these as non-functions

I used -stdc++98 in the g++ command line to emphasizes that this is just old school C++ namespace semantics at work.

Inflect answered 13/7, 2017 at 18:35 Comment(9)
But the GCC implementation of cmath puts them into the std namespace with using ::abs; etc. The deprecation tag carries over; if you replace the return 1; with return std::abs(-1); it still warns about deprecation.Ardy
@DanielH I see: it is bringing them in with using and then just adding some overloads. Ouch! So the primary one inherits the warning and only the overloads are safe. That's pretty stinky.Inflect
So unless the g++ headers are sanitized, it's down to requiring a warning when ::abs or abs is used, but not when std::abs is used, even when std::abs is an alias for the previous two.Inflect
Yeah, that’s what I’d like. I don’t know if it exists, though.Ardy
Maybe worth noting that abs(int) is in cstdlib, not cmathPalladous
@Palladous Until C++17, yes, and my example is a little artificial for including both. But in a real program, some C++ header file probably includes <cstdlib> even if you don’t need to do it explicitly.Ardy
@danielh: library headers cannot include other library headers unless specifically noted by the standard.Palladous
@Palladous Yes, they can. I think that’s a rule for C, not it isn’t one for C++.Ardy
@DanielH: Ok, I take it back. Seems like it is highly likely that cstdlib will be included, at least with the Gnu C++ library (in which <string>, amongst others, indirectly includes <cstdlib>). But I made the original comment about cstdlib vs. cmath because of the wording in the first three paragraphs of this answer.Palladous
P
0

This code will let you detect whether the trap exists in a particular environment:

double (*)(double) = &::abs; // fails if you haven't included math.h, possibly via cmath

But it won't help you spot the places you fall into the trap.

Pretend answered 13/7, 2017 at 18:29 Comment(0)

© 2022 - 2024 — McMap. All rights reserved.