On Thu, Nov 18, 2010 at 4:43 PM, Peter Stuge <pe...@stuge.se> wrote:
> Andrew Leech wrote:
>> > (A note that you can send patches easily by first committing and then
>> > using the git send-email command.)
>>
>> Ok I'm still pretty new to git, so by committing it just adds changes
>> to the local repository not the mainline one?
>
> Exactly. Every git working copy is a fully worthy repository clone.
> (Hence the git clone command.)
Thanks for the git helper, that's a really concise overview of the diff usage.
>
>
>> I'm in no way certain about quiet having a dash, it just seems more
>> consistent to me to have all switches with a dash, more like standard
>> *nix programs. On the other hand it's also important to keep
>> consistency within openocd to try to make it easier to use.
>
> The backwards compatibility is something I think is particularly
> important, even if the SVF support is not the most frequently used
> part of openocd.
>
I agree on the backwards compatibility, hence I made the dash on quiet
optional, it accepts quiet and -quiet. But yeah it does look cleaner
with no dash I feel.
>
>> I'm also not too concerned whether -help stays in there, as as
>> mentioned yes usage gets printed if you have an incorrect command
>> line anyway. I'm not sure if help as a command is common in any
>> other modules. Either way I'm more than happy for you or anyone to
>> modify the command line layout / behavior to keep it relevant.
>> For now I've changed it to usage: svf [-tap device.tap] <file>
>> [quiet], I do think this looks cleaner.
>
> That's really good. That means that the change has a minimal and
> clean contact surface with everything previous.
>
>
>> >> +static int svf_adjust_array_length(uint8_t **arr, int orig_bit_len, int
>> >> new_bit_len, int set);
>> >
>> > I would suggest adding a function with a new name that takes the
>> > extra parameter, and renaming the existing function, and adding the
>> > parameter to it. Then add a one-line function with the original name
>> > that calls the new function with the last parameter set to 0. This
>> > way, each previous call site for the existing function does not need
>> > to be touched by this patch. The new call sites invoke the renamed
>> > function and specify values as needed for the extra parameter.
>> >
>>
>> Once again this was the way I was initially implementing it and it
>> gave some minor issues and I then thought it seemed wasteful to add
>> another function just to modify the functionality of an existing
>> function. But yeah, going and modifying all calls to an existing
>> function is pretty messy too.
>
> Adding a function which is basically just a wrapper is admittedly a
> little wasteful, but I think the benefit of keeping the change small
> and clean is important enough to outweigh that waste. Your new
> solution is even better though.
>
>
>> > command_print(CMD_CTX, "open(\"%s\"): %s", CMD_ARGV[i], strerror(err));
> ..
>> Yeah that error reporting makes a lot of sense, added. Now if you
>> specify a folder rather than a file you get "permission denied", much
>> better.
>
> All right!
>
>
>> I'm testing on Windows 7 (cygwin) x64 (x32 openocd binaries) and
>> haven't had any issues with the file open. I've got a linux vm I
>> could have a got at compiling it on, probably should give it a test.
>
> If you like to, but I don't think it's needed. I was mostly worried
> about how this would behave on Windows, and since you have tested
> there I don't think the changes need more testing.
>
>
>> > Watch out for trailing whitespace, in the last line quoted above.
>>
>> Ah now I see what you mean about the trailing whitespace, I never
>> recognized this before. I think I've fixed it up, haven't noticed any
>> in the new patch.
>
> git diff can colour code it's output, and make whitespace errors very
> visible. The colouring works by default in Linux, but unfortunately
> not in Windows. Maybe this is sufficient:
>
> git config --global color.ui true
> git config --global core.whitespace trailing-space,space-before-tab
>
> Also make sure to let git know that it should not permit any Windows
> line-endings into the codebase:
>
> git config --global core.autocrlf true
>
>
<snip>
>> - if ((svf_fd = open(CMD_ARGV[0], O_RDONLY)) < 0)
>> +
>> + if (svf_fd == NULL)
>
> Sorry, this isn't right. The fd is an int, open() returns < 0 on
> failure, but NULL is a void *, which normally is defined as 0. svf_fd
> needs to be initialized to -1 in order to have a reliable test of
> whether a file was successfully opened or not, and the condition here
> chould then check if svf_fd == -1.
>
yep I noticed this myself, it's getting fixed. I've also found
switching to fopen and improving the file read function has given a
slight (10%) speed improvement, so now NULL will be correct :)
>
>> + command_print(CMD_CTX, USAGE);
>
> Again the "usage :" string. It's frequent enough that I think it
> makes sense to define a second USAGE variant which combines the
> "usage: " string with the actual USAGE string.
>
>
>> +static int svf_set_padding(struct svf_xxr_para *xxr_para_tmp, int length,
>> unsigned char tdi)
>> +{
>> + int error = ERROR_OK;
>> + error |= svf_adjust_array_length(&xxr_para_tmp->tdi,
>> xxr_para_tmp->len, length);
>> + memset(xxr_para_tmp->tdi, tdi, (length + 7) >> 3);
>
> Nice solution with the memset() within this function. Please change >> 3
> into / 8 though, it is slightly easier to read and the compiler will
> do the optimization for us. A very minor detail is also that if
> xxr_para_tmp would have a shorter name, maybe just para, then all
> lines could make it in under 80 characters wide. 80 is in no way a
> hard requirement, but it is the width of my windows, and maybe many
> others, so I try to stay under 80 if I can.
>
>
>> + if (!svf_quiet)
>> + {
>> + if (skipped)
>> + {
>> + LOG_USER("%s (skipped)", svf_command_buffer);
>
> So far there is only the -tap reason to skip a command, but perhaps
> the flag should be named more specifically, like skipped_tap, and the
> message could mention -tap somehow?
>
Hmm, yeah I'll have a think about this one, I've damaged the output
here already somehow, it's only printing each lines' command and not
their arguments. I'll have to fix it too.
>
>> + .usage = "[-tap device.tap] <file> [quiet]",
>
> It would be nice to reuse the USAGE define here as well.
>
Yeah I was thinking this too, just hadn't thought of rolling the whole
print usage line earlier into a define, doing this makes it neat
enough to just have the string here in one define, and the rest of a
print command as a second one.
I'm also most of the way through adding a progress output to svf too,
as another configurable option. I've found that when programming can
take a few minutes it's really nice to have an idea of how it's going.
But now I doubt I'll get much chance to finish it and the rest of
these suggestions until next week, I'll chime back in when they're
done.
Cheers,
Andrew
_______________________________________________
Openocd-development mailing list
Openocd-development@lists.berlios.de
https://lists.berlios.de/mailman/listinfo/openocd-development
