c-ridgway / cpp-feather-ini-parser

Header only, simple, fast, templated - portable INI parser for ANSI C++.
MIT License
47 stars 14 forks source link

Key/Val pairs not allocated in copy construction? #5

Closed marcbertola closed 5 years ago

marcbertola commented 6 years ago

Hello, I was experimenting with feather INI when I came across an issue related to the copy constructor. The Key/Value pairs are in a dynamically allocated map, but I don't see the copy constructor making a new copy (i.e. allocating a new map). Thus when the original is deleted, the key/val pairs of the copy are also deallocated. The following scenario illustrates the problem. In my environment, it crashes in Debug mode, and prints no key/val pairs in Release mode:

#include <feather_ini/INI.h>
#include <iostream>

void printIni(INI<> & ini) { // Reference, copy constructor not called
    for (auto & section : ini.sections) {
        std::cout << "[" << section.first << "]" << std::endl;
        auto & keyValMap = *(section.second);
        for (auto & keyVal : keyValMap) {
            std::cout << keyVal.first << "=" << keyVal.second << std::endl;
        }
    }
}

int main(int argc, char * argv[]) {
    auto origIni = new INI<>("C:\\Windows\\system.ini", true);
    std::cout << "Original: ==============================" << std::endl;
    printIni(*origIni);

    auto copyIni = new INI<>(*origIni);
    delete origIni; // This deletes the key/val pairs shared by orig and copy

    std::cout << std::endl;
    std::cout << "Copy: ==============================" << std::endl;
    // Only displays the sections, not the key/val pairs, or crashes
    printIni(*copyIni);
}
Turbine1991 commented 6 years ago

Hi Marcbertola, I highly recommend viewing the example and documentation in detail.

There is currently no constructor to copy the ini's contents: auto copyIni = new INI<>(*origIni);

The only constructors available are:

ini(filename, doParse)
ini(data (AS BYTE ARRAY), dataSize (OF BYTE ARRAY), doParse)

Also to delete a key, do NOT use the delete keyword. That is for deleting a POINTER's referenced object from memory. I should probably add an 'erase' function for section.

This is how you can currently achieve key removal: ini["section"].erase("key");

Hopefully this clears up a few details, feel free to ask another question.

marcbertola commented 6 years ago

Thanks for the response.

I have no problems using the library, I just figured it would be helpful to provide feedback to you about the copy constructor. While you may not have defined one, please keep in mind that the compiler automatically generates a default copy constructor if you don't provide one. The problem in this case it that the default one won't make a fresh copy of the key/value pairs and this can lead to problems.

It is not unreasonable for users of the library to expect copy construction to work unless you delete the constructor explicitly. A user of the library could accidentally trigger the copy constructor. Consider making a std::vector of INI objects, for example.

Anyway, as I mentioned, this is just feedback to help you out.

Turbine1991 commented 6 years ago

Feel free to make a deep copy, I'd happily push it.

Turbine1991 commented 5 years ago

I've finally re-written this parser. Everything has been addressed.

There are no line length limits.

The following is now parsed without errors: -Comments (// and #) -Semi-colons at the end of a key value pair -Indentation

The following may be applied when saving: -Pruning empty sections/keys value pairs -Padding between section blocks -Spaces around section value -Spaces inside key value pairs -Indented key value pairs -Semicolons appended to key value pairs