13

I am refactoring some C code and performing unit testing on the factored out parts (using Google Test). One snippet was used several times in a loop, so in order to expose it to testing I factored it out as an inline function in a header file demo.h which also includes declarations of some other non-inline functions. A simplified version is as follows:

#ifndef DEMO_H_
#define DEMO_H_
#ifdef __cplusplus
extern "C" {
#endif
inline void print_line(FILE* dest, const double * data, int length) {
    for (int s = 0; s < length; s++)
        fprintf(dest, "%lf ", data[s]);
    fprintf(dest, "\n");
}
#ifdef __cplusplus
}
#endif
#endif /* MK_H_ */

My test code

#include "gtest/gtest.h"
#include "demo.h"
#include <memory>
#include <array>
#include <fstream>

TEST (demo, print_line) {
    std::array<double,4> test_data = {0.1, 1.4, -0.05, 3.612};

    const char* testfile = "print_line_test.txt";
    {
        auto output_file = std::unique_ptr<FILE, decltype(fclose)*>{
            fopen(testfile, "w"), fclose };
        print_line(output_file.get(), test_data.data(), test_data.size());
    }

    std::ifstream input(testfile);
    double dval;
    for(const auto& v: subsequence_data) {
        input >> dval;
        EXPECT_EQ (v, dval);
    }
    EXPECT_FALSE (input >> dval) << "No further data";
}

int main(int argc, char **argv) {
  ::testing::InitGoogleTest(&argc, argv);
  return RUN_ALL_TESTS();
}

This code compiles and runs fine under MinGW g++ 4.8.1 with -std=gnu++0x.

The original C code then makes of use of this function. A simplified version would be the following:

#include "demo.h"

void process_data(const char* fname, double ** lines, int num_lines, int line_length) {
    FILE* output_file = fopen(fname, "w");
    for (int i=0; i<num_lines; ++i) {
      print_line(output_file, lines[i], line_length);
    }
}

However, when I try to compile my C code using MinGW GCC 4.8.1 with -std=c99, I get the following warning:

warning: 'fprintf' is static but used in inline function 'print_line' which is not static [enabled by default]

I also get a subsequent error, which may be related:

undefined reference to `print_line'

Changing the signature in the header to static inline void print_line ... appears to fix the problem. However, I don't like not understanding the cause of the issue. Why did the lack of static not affect the C++ test? And what did the error regarding fprintf actually mean?

Billal Begueradj
  • 20,717
  • 43
  • 112
  • 130
beldaz
  • 4,299
  • 3
  • 43
  • 63
  • 4
    First problem, `-std=c99` is wrong because **THIS IS NOT** [tag:c]. – Iharob Al Asimi Jan 25 '16 at 21:51
  • 1
    @iharob I believe you are quite wrong there. The test code is C++, but the header file is C and is used in code that is written in C. I think I have made this very clear in my question. – beldaz Jan 25 '16 at 21:57
  • 1
    inline functions are not used unless they are called somewhere, then the call is replaced by the function body (as it does with macros). so in your code `print_line` points to nowhere. – milevyo Jan 25 '16 at 21:57
  • @milevyo The code is used in my original C program that I am refactoring. I will expand the question to make this clearer. – beldaz Jan 25 '16 at 21:59
  • Why do you think you get an *undefined reference*? You really need to read about the differences between these languages. – Iharob Al Asimi Jan 25 '16 at 22:00
  • you can not reference inlined functions from outside the module – milevyo Jan 25 '16 at 22:05
  • @iharob I have added a few lines to make the problem clearer. Please indicate if you think the problem needs further revision. – beldaz Jan 25 '16 at 22:13
  • @milevyo Could you explain why `static` fixed the problem? I think that's the confusing part for me. Also, what do you mean my "module" here? Do you mean a compilation unit? – beldaz Jan 25 '16 at 22:17
  • you are right about module. but i will not explain why static resolves the problem, [this](http://www.greenend.org.uk/rjk/tech/inline.html) link do explain it clearly. – milevyo Jan 25 '16 at 22:23

2 Answers2

9

Without static, you are allowing a C99 compiler to create both a function with external linkage (defined in a single place), but also separate inline code in every translation unit that includes the file. It can use any function it likes, unless you explicitly decide between static or extern.

One requirement of such functions can be seen in C99 Draft 6.7.4.3:

An inline definition of a function with external linkage shall not contain a definition of a modifiable object with static storage duration, and shall not contain a reference to an identifier with internal linkage.

This makes sense, because compiler wants this function to behave equally, regardless of how it chooses to implement it.

So, in this case the compiler is complaining that your non-static inline function is calling a different function which is static, and it isn't sure that this other function (fprintf) doesn't mutate static storage.

vgru
  • 49,838
  • 16
  • 120
  • 201
  • 2
    Thanks, that specifically answers the issue I was observing, and helpfully interprets that troubling warning. – beldaz Jan 25 '16 at 22:48
4

First of all, the behaviour of inline , particularly with respect to linkage of the symbols involved, is different in C to C++. (And also differs between ISO C and GNU C).

You can read about the C version here.

If you try putting a function body in a header that is included from both C and C++ (within the same project) then you are opening a real can of worms. That situation isn't covered by either language standard. In practical terms I would treat it as an ODR violation because the C version of the function is different to the C++ version.

The safe thing to do is to include only the function prototype in the header, and have the function body in one of the non-header source files.

Community
  • 1
  • 1
M.M
  • 138,810
  • 21
  • 208
  • 365
  • 1
    Thanks, the linked SO answer is an excellent resource in itself. Putting the function into a separate compilation unit seems like the best approach, and I'll leave inlining for the LTO. – beldaz Jan 25 '16 at 22:29