Re: [ANN] List_Image v0.2.0

[Lionel Draghi <lionel.draghi@gmail.com>]

Le 31/01/2018 à 08:27, emmanuel... a écrit :
>> List_Image is a small helper to print the content of Ada predefined containers, available here : https://github.com/LionelDraghi/List_Image
> 
> 
> Nice !
> 
> And well documented :-)

Thanks! :-)

...
> Some comments on the code:
> 
>    * EOL should be defined different when on Windows. I used to test whether the GNAT.OS_Lib.Directory_Separator is '\', perhaps there's a better way nowadays. For sure, a "Is_Windows" constant would be useful somewhere.
> 
>       EOL : constant String := (if Is_Windows then ASCII.CR & ASCII.LF else ASCII.LF);

I defaulted EOL to Windows CRLF, because it's seems to work well on my Linux box also. But note that it can be overridden when instantiating Cursors.
I don't now a standard way to get the good line terminator.

>    * Cursors_SIgnature is what I was calling a Forward_Cursor in the traits containers. So there's some code sharing to be done here :-) However, the profile of Has_Element and Next, in my case, was also receiving the container as a parameter, so that they could be compatible with the formal containers distributed with GNAT, and used in SPARK. It is easy to ignore that extra parameter for the standard Ada containers, but not possible to invent it if not given.

I am always open to Identifiers changes to have a more coherent and comprehensive naming scheme.

For Next and Has_Element, changing the profile would prevent the easy instantiation with Ada containers. 
If we want the same simplicity with another container collection, I suppose there's no way out, we need to provide more versions of this Cursor generic. 

> For some reason, I was also passing No_Element and "=", but without looking at the code in more details, my first thought now is that these should not be there, so your approach is lighter and likely better.

My hesitation was on the Has_Element parameter. 
The code was simpler when using the Length function instead. 
I remembered that for some reason you decided to provide your own Ada.Containers.Count_Type in your Container, so I choose to stick to First/Next/Has_Element instead of First/Next/Length. 
The body code is more complex, but the component has no dependency (except Unbounded_String in the body), and that's better for reusability.

>    * It would be more efficient to use GNATCOLL.Strings instead of Unbounded_Strings (a lot more effort was done for performance there), but then it adds a dependency on a third party library. GNATCOLL.Strings is also using some kind of traits-based approach (and I have a version locally that uses traits to decide whether to use atomic counters, non-atomic counters, or no counter at all (and thus no copy-on-write). Again in the spirit of sharing a signature-based library somewhere

Yes, I prefer to avoid non standard dependencies.
But we could provide an alternate body. 

>      By the way, using  'Tmp := Tmp & "...";' is inefficient compared to 'Append (Tmp, "...")` so I would recommend the latter.

OK, I will update the code.

>     * I see you finally gave up on using the iterable as formal parameters. I also had very limited success with that. As a result, the generic algorithms have to be written using explicit cursors, but since they are reused easily, that's not such an issue.
Yes, this is my conclusion for this POC! :-)

Thanks again Emmanuel for all the useful inputs.