Re: [PATCH] Initial policyrep patch

[James Antill <jantill@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 3200 bytes --]

On Mon, 2007-06-25 at 14:28 -0400, Karl MacMillan wrote:
> Initial patch to create a new policyrep branch using C++. This patch includes basic classes for
> representing policy (Node and Parent), a few policy objects, a bison parser, a boost::python
> binding, and basic test infrastructure.
> 
> Signed-off-by: User "Karl MacMillan <kmacmillan@mentalrootkit.com>"

 So I suck at C++, so feel free to ignore any of these comments that are
stupid :)

> +namespace policyrep {
> +
> +	class location;
> +
> +	class Parser {
> +	public:
> +		Parser();
> +		virtual ~Parser();
> +
> +		// scanner
> +		void scan_begin();
> +		void scan_end();
> +		bool trace_scanning;
> +
> +		// Parser
> +		Module* parse(const std::string& f);

 Why are you returning a pointer here, this implies that it can return
NULL ... but it can't, it throws an exception if there's an error.

> +		std::string file;

 Do you really want public members like this?

> +		bool trace_parsing;
> +		Module* pmodule;
> +
> +		// error handling
> +		virtual void error(const policyrep::location& l, const std::string& m);
> +		virtual void error(const std::string& m);
> +	};
[...]
> +	struct PolicyImpl;
> +	class Policy : public Parent {
> +	public:
> +		Policy(bool mls=false);
> +		Policy(const Policy& other);
> +		virtual ~Policy() { };
> +		void operator=(const Policy& other);

 Is it normal to have only a virtual dtor? My understanding was you
generally wanted ctor, dtor and op= to have the same overidability.

> +		bool get_mls() const;
> +		void set_mls(bool val);

 Isn't this better done with polymorphism? Like:

                 bool mls() const;
                 void mls(bool);

> +                virtual void output(std::ostream& o, enum OutputStyle style=DEFAULT_OUTPUT) const;
> +	protected:
> +		PolicyImpl* impl;
> +	};
> +	typedef boost::shared_ptr<Policy> PolicyPtr;

[...]             
> +                std::string get_name() const;
> +                void set_name(std::string name);
> +                std::string get_version() const;
> +                void set_version(std::string version);

 Don't you want to be returning a const reference like:

                  const std::string &name(void) const;

...AIUI this automatically converts to a std::string via. copy in the
cases you need a writable version. Dito. the set version, but just for
the speed increase (Ie. the internal member is still a plain
std::string).

> +       Module::Module() : impl(new ModuleImpl) { }
> +
> +       Module::Module(const Module& other)
> +               : Parent(), impl(new ModuleImpl)
> +       {
> +               *impl = *other.impl;
> +       } 

 So you are allocating memory to a pointer impl, but the Policy dtor
does nothing?

> +                virtual void output(std::ostream& o, enum OutputStyle style=DEFAULT_OUTPUT) const;
> +        protected:
> +		ModuleImpl* impl;
> +        };
> +	typedef boost::shared_ptr<Module> ModulePtr;
[...]

> +	void Policy::operator=(const Policy& other)
> +	{
> +		*impl = *impl;	
> +	}

 This looks very wrong.


-- 
James Antill <jantill@redhat.com>

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 189 bytes --]