Project

General

Profile

Actions

Bug #2299

closed

std::snprintf not found in buggy STL implementations

Added by Alex Afanasyev over 9 years ago. Updated almost 9 years ago.

Status:
Closed
Priority:
Normal
Category:
Build
Target version:
Start date:
Due date:
% Done:

100%

Estimated time:

Description

At least with cygwin, does not properly define std::snprintf, but C's version ::snprintf is still available.

I'm proposing changing instances of std::snprintf with the following:

// inside function body
using namespace std;
snprintf(...);

Files

snprintf.patch (1.36 KB) snprintf.patch Mathias Gibbens, 04/13/2015 11:08 AM
Actions #1

Updated by Junxiao Shi over 9 years ago

I disagree with the proposed solution. using namespace should be avoided.

  1. check for std::snprintf during configure step
  2. in common.hpp, declare std::snprintf as a function that forwards its arguments to ::snprintf
Actions #2

Updated by Alex Afanasyev over 9 years ago

This technique is used in many places in boost libraries and I don't see problem with that.

We can check during configure stage, but then it is just more complicated to use, while there is a simple solution that should solve all the problems.

Actions #3

Updated by Junxiao Shi over 9 years ago

What about using ::snprintf everywhere?

Actions #4

Updated by Alex Afanasyev over 9 years ago

I could be ok with that

Actions #5

Updated by Alex Afanasyev over 9 years ago

I decided to reject this task, as it is just a really buggy cygwin implementation. Code can be compiled with addition of a few defines.

Actions #6

Updated by Alex Afanasyev over 9 years ago

  • Status changed from New to Rejected
Actions #7

Updated by Alex Afanasyev over 9 years ago

  • Status changed from Rejected to New

I'm reopening this, as android compilation has exactly the same problem, and unfortunately I cannot fix it with a few extra defines.

Actions #8

Updated by Davide Pesavento over 9 years ago

Just use the C version, as it was before we moved to C++11.

Actions #9

Updated by Alex Afanasyev over 9 years ago

  • Assignee set to Alex Afanasyev
  • Target version set to v0.3

Will replace with C version of snprintf

Actions #10

Updated by Alex Afanasyev about 9 years ago

  • Status changed from New to In Progress
Actions #11

Updated by Alex Afanasyev about 9 years ago

  • Status changed from In Progress to Code review
  • % Done changed from 0 to 100
Actions #12

Updated by Alex Afanasyev about 9 years ago

  • Status changed from Code review to In Progress
  • % Done changed from 100 to 70

Similar fix needs to be done in NFD codebase. Will submit new patch later.

Actions #13

Updated by Alex Afanasyev about 9 years ago

  • Status changed from In Progress to Code review
  • % Done changed from 70 to 100
Actions #14

Updated by Junxiao Shi about 9 years ago

  • Status changed from Code review to Closed
Actions #15

Updated by Mathias Gibbens almost 9 years ago

I hit this problem today using OpenWRT's build toolchain (gcc 4.8.3) when cross-compiling NFD 0.3.1. There is a usage of std::snprintf in ./daemon/face/ethernet-face.cpp that is causing the same problem. Alex's previous patch only fixed the issue in ./core/logger.cpp. There are no other occurrences of snprintf in the NFD 0.3.1 code.

Can we get a fix for this included in the next NFD release?

Actions #16

Updated by Junxiao Shi almost 9 years ago

  • Status changed from Closed to Feedback
  • Target version changed from v0.3 to v0.4

Reopening as requested by a Mathias.

Actions #17

Updated by Alex Afanasyev almost 9 years ago

@Mathias Can you submit your patch for the code review?

Actions #18

Updated by Mathias Gibbens almost 9 years ago

Here is a patch against the current NFD master branch that fixes the issue. It is modeled after Alex's previous two patches.

Actions #19

Updated by Alex Afanasyev almost 9 years ago

@Mathias, can you please push your patchset to http://gerrit.named-data.net/?

Actions #20

Updated by Mathias Gibbens almost 9 years ago

@Alex, I don't have an account setup for the project's Gerrit. This is such a small patch that I'd like to not go through the trouble of setting up another account just to commit it. I'd be fine with either you or Junxiao committing it.

Actions #21

Updated by Alex Afanasyev almost 9 years ago

  • Status changed from Feedback to Code review
Actions #22

Updated by Junxiao Shi almost 9 years ago

  • Status changed from Code review to Closed
Actions

Also available in: Atom PDF