current community

your communities

more stack exchange communities

Stack Exchange
tour help
Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 7 down vote favorite
1

Abstract

I have little C/C++ skills. I've learned programming mainly with Java, and so I chose C++ (targeting C++11) as the language to learn better. In my current C++ project, I have to use the discount C library for its Markdown functions.

To explain briefly my code: the member variable std::string markdown_ is the Markdown I want to "compile" to HTML. To do so, I have to use the following functions from discount:

  • MMIOT *mkd_string(char *string, int size, int flags);
  • int mkd_compile(MMIOT *document, int flags);
  • int mkd_document(MMIOT *document, char **doc);
  • mkd_css and mkd_toc are alike mkd_document

Questions

  1. As you see, I need to copy the markdown_ string and make it a char*. To do so, I used a std::vector as suggested in Converting an std::string into a char array, but I would like to know if doing it this way is appropriate in my case, or if there are other ways to achieve that.

  2. MMIOT *mkd_string(char *string, int size, int flags); is defined in the main page to return null on error. Is it correct to use nullptr like I do in the following code to check the return value?

  3. Do you have suggestions to improve this code, or does it looks good?

Code extract

const std::string Markdown::error_msg_mkd_string = "libmarkdown failed to "
    "parse the following Markdown:\n";

void Markdown::compile()
{
    MMIOT* doc;
    int size;
    char *cstr_buff;
    int flags = MKD_TOC|MKD_SAFELINK|MKD_EXTRA_FOOTNOTE;

    std::vector<char> buff(markdown_.cbegin(), markdown_.cend());
    doc = mkd_string(&buff[0], buff.size(), flags);

    if(doc == nullptr)
        throw std::runtime_error(
            std::string(error_msg_mkd_string)
                .append(&buff[0], buff.size())
            );

    mkd_compile(doc, flags);

    // It is not a bad practice to reuse 'size' and 'cstr_buff' like in the
    // following code, or is it?
    size = mkd_css(doc, &cstr_buff);
    css_.assign(cstr_buff, size);

    size = mkd_document(doc, &cstr_buff);
    html_.assign(cstr_buff, size);

    size = mkd_toc(doc, &cstr_buff);
    toc_.assign(cstr_buff, size);

    mkd_cleanup(doc);
}
share|improve this question
    
One const could save you a lot of head-ache. In the function MMIOT *mkd_string(char *string, int size, int flags); is char *string modified/changed in this function at all? –  ahenderson Oct 17 '13 at 15:26
    
I just checked the discount library source code, and mkd_string takes indeed a const char* (see mkdio.c) but the function is declared without const in the library documentation. @ahenderson –  kuroneko Oct 18 '13 at 10:12
    
in that case simply use mkd_string(markdown_.c_str(), markdown_.size(), flags);. String c_str() returns the string as a const char *. –  ahenderson Oct 18 '13 at 13:00

2 Answers 2

up vote 2 down vote accepted
  1. If mkd_string accepts a const char*, then you are better off using a std::string and its method c_str() to pass the value to the function. Otherwise your solution is correct.

  2. Yes. See this answer for the why. Or build this code (tested with g++ -std=c++0x, gcc version 4.6):

    #include <iostream>
int main()
{
    if (NULL == nullptr)
        std::cout << "NULL == nullptr\n";   
}

  3. Declare the variable immediately before using it, possibly also defining it. This reduces its scope to the minimum, helps the reader/maintainer of the code understanding what the variable is for, and avoids terrible errors deriving from forgetting to initialize variables. (This should also provide you with some guideline for the question about re-using the same variable twice in the same block.)

share|improve this answer
up vote 0 down vote

std::string has a built-in function (c_str) to convert the std::string to a const char*. It returns a pointer to the internal character string (terminated by a 0).

std::string str;
const char* cstr = str.c_str();

If you need a non-const char*, do not cast out the const. That will cause a bug. Instead, copy it to a new non-const char*.

char* cstrCopy = new char [str.length()+1];
std::strcpy (cstrCopy, str.c_str());
share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.