Skip to content

REFAC: remove unnecessary calls to c_str (2nd attempt)

Victor Poughon requested to merge 513-string-cstr into develop

Summary

  • Add 170 missing #include <string> in headers declaring std::string variables, class members or return types.
  • Remove 755 unnecessary calls to c_str where a string overload exists

Rationale

Issue #513 (closed) points out that calls to c_str are getting more dangerous with C++11 and can causes crashes. We need to identify incorrect uses. But the problem is that there are currently >1500 calls to c_str in OTB, making it difficult to find those potential dangling pointers. However in a lot of cases they are not necessary because there exists a function that takes a std::string directly. We are further helped by C++11 which added some constructors and methods that take strings, where previously only the const char* version existed.

This MR removes 755 calls to c_str which can simply be deleted and need no other refactoring, because a string overload exists. Other instances will require a bit more manual refactoring so they can be done in a future MR. This is only for the very easy ones that can be fixed with sed.

Adding missing #include <string> is necessary to avoid build errors on MSVC (see previous attempt !164 (closed)). I think some compilers implicitly include parts of <string> especially operator <<, so it currently works but it is bad practice. Headers declaring std::string variables or class members should include it.

Implementation Details

For the first step, to find guilty header files, I used:

grep -l "^ *std::string" $(grep -L "#include <string>" $(find . -type f -name "*.h"))

which finds all files containing "std::string" at the beginning of a line (usually a member or variable declaration), but not "#include ". And then this script to add the includes (plus some manual editing):

#!/usr/bin/env python3

import re
import argparse

def fix_file(filename, header):

    with open(filename, "r") as f:
        content = f.read()

    matches = list(re.finditer(r"(#include .*\n)\n", content))
    if len(matches) == 0:
        print("no include!")
        sys.exit(-1)

    pos = matches[-1].end(1)
    open(filename, "w").write(content[:pos] + "#include <{}>\n".format(header) + content[pos:])

if __name__ == "__main__":
    parser = argparse.ArgumentParser()
    parser.add_argument('--header', type=str, required=True)
    parser.add_argument('files', type=str, nargs='+')
    args = parser.parse_args()

    for filename in args.files:
        fix_file(filename, args.header)

And for the second step:

#!/bin/bash

set -eou pipefail

# Use C++11 sstream constructor from string
# Exclude 'ThirdParty' because it uses ossimFilename type
find Modules/ Examples/ -not -iwholename "*ThirdParty*" -type f -print0 | xargs -0 sed -i -E "s/std::ifstream (.*)\((.*)\.c_str\(\)/std::ifstream \1\(\2/"
find Modules/ Examples/ -not -iwholename "*ThirdParty*" -type f -print0 | xargs -0 sed -i -E "s/std::ofstream (.*)\((.*)\.c_str\(\)/std::ofstream \1\(\2/"

# Use C++11 ::open(const std::string&) of ifstream, ofstream, etc.
# Exclude ThirdParty because of ossimFilename type
find Modules/ Examples/ -not -iwholename "*ThirdParty*" -type f -print0 | xargs -0 sed -i -E "s/\.open\((.*).c_str\(\)/\.open\(\1/"

# No need for c_str when outputing to std::cout and such
find Modules/ Examples/ -type f -print0 | xargs -0 sed -i -E "s/\.c_str\(\) ?<</ <</"

# Use itk::ExceptionObject::SetDescription and SetLocation string versions
find Modules/ Examples/ -type f -print0 | xargs -0 sed -i -E "s/e\.SetDescription\((.*)\.c_str\(\)\)/e\.SetDescription\(\1\)/"
find Modules/ Examples/ -type f -print0 | xargs -0 sed -i -E "s/e\.SetLocation\((.*)\.c_str\(\)\)/e\.SetLocation\(\1\)/"

# Use itksys::SystemTool:: string versions
find Modules/ Examples/ -type f -print0 | xargs -0 sed -i -E "s/itksys::SystemTools::(.*)\((.*)\.c_str\(\)/itksys::SystemTools::\1\(\2/"
# Exclude otbLogger because it uses GetCurrentDateTime which does not have a string version
git checkout --  Modules/Core/Common/src/otbLogger.cxx
# Exclude test/otbOGRLayerStreamStitchingFilter.cxx because there's a double use which is not compatible with the regex
git checkout -- Modules/Segmentation/OGRProcessing/test/otbOGRLayerStreamStitchingFilter.cxx

Additional notes

The python script to add missing includes could be used for other symbols if needed (vector, etc.)

Copyright

The copyright owner is CNES and has signed the ORFEO ToolBox Contributor License Agreement.


***Check before merging:*** - All discussions are resolved - At least 2 👍 votes from core developers, no 👎 vote. - The feature branch is (reasonably) up-to-date with the base branch - Dashboard is green - Copyright owner has signed the ORFEO ToolBox Contributor License Agreement

Merge request reports