Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

INIReader: change variable visibility from private to protected #165

Merged
merged 1 commit into from
Apr 6, 2024

Conversation

meiyasan
Copy link
Contributor

@meiyasan meiyasan commented Mar 26, 2024

@benhoyt I am proposing to change variable visibility allowing for full feature inheritance.

@benhoyt
Copy link
Owner

benhoyt commented Mar 26, 2024

Hi @xkzl I'm not opposed to this, though it's been a while since I've done C++. Are there any other implications other than making these fields accessible to subclasses? Can you please provide a small example of how this would be used?

@meiyasan
Copy link
Contributor Author

meiyasan commented Mar 30, 2024

Hi @benhoyt, sorry for my late reply. You are right, there is no other implications than making fields and methods accessible to subclasses. Unless, the variable content is very specific (void* variable manipulation that only the mother class knows about for instance), it is okay to turn this as protected.

As an example, I would mention a custom INIReader I have been designing to retain the section and fields information.
Her eis the header file:


#ifndef __INIREADER_H__
#define __INIREADER_H__

#include <map>
#include <set>
#include <string>
#include <INIReader.h>

class INIReaderPlus: public INIReader 
{
public:

    std::set<std::string> GetSections() const;
    std::set<std::string> GetFields(std::string section) const;

protected:
    int _error;
    std::map<std::string, std::string> _values;

    std::set<std::string> _sections;
    std::map<std::string, std::set<std::string>*> _fields;
    static std::string MakeKey(std::string section, std::string name);
    static int ValueHandler(void* user, const char* section, const char* name,
                            const char* value);
};

#endif

I can take advantage of your modifications and also customize the class, without protected elements I would have to rewrite the whole implementation (that I haven't studied). Doing so it helps factorizing.

@benhoyt
Copy link
Owner

benhoyt commented Mar 30, 2024

@mayak-dev @jcormier @vrresto @evorw You folks are the last few people who've touched INIReader (the C++ class) -- any objections to making these internal fields protected rather than private? If not I'll go ahead and merge this later this week.

@mayak-dev
Copy link
Contributor

mayak-dev commented Mar 30, 2024

Looks good to me!

@jjcf89
Copy link

jjcf89 commented Mar 30, 2024

Seems reasonable

@benhoyt
Copy link
Owner

benhoyt commented Apr 6, 2024

Thanks for your replies, @mayak-dev and @jjcf89. Merging.

@benhoyt benhoyt changed the title Update INIReader.h INIReader: change variable visibility from private to protected Apr 6, 2024
@benhoyt benhoyt merged commit ec8539d into benhoyt:master Apr 6, 2024
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants