Re: [PATCH 1/5] staging/pi433:Removed Coding style issues from pi433_if.h and other dependencies arising from it.

[Marcus Wolf]

Hi!
 
Ok. Didn't know that I have to check for such stuff.

So far i was just checking the code changes, not the style of the patch itself.

Will try to be more strict...

Is it mandatory, that I compile the code, or is it ok, if I do the review "just"
by reading? 
Reason for the question: If reading is ok, too, I can do a review of a simple
change,
even when I am abroad at a customer (my customers do not deal with linux, just 
comercial stuff). With compiling, I can only do them at home...

Cheers,

Marcus



> Dan Carpenter <dan.carpenter@xxxxxxxxxx> hat am 2. August 2017 um 10:34
> geschrieben:
>
>
> On Wed, Aug 02, 2017 at 10:08:04AM +0200, Wolf Entwicklungen wrote:
> > Reviewed-by: Marcus Wolf <linux@xxxxxxxxxxxxxxxxxxxxx>
> >
> > Just reviewed, not tested.
> > As far as I can see, there is no technical issue with this patch.
>
> You need to be a bit more strict in your reviews... There were a few
> obvious problems in this patchset. These are show stoppers:
> 1) Breaks git bisect
> 2) Doing multiple things in the same patch
> 3) No changelog
>
> >
> > I prefer the names of the enumerations in camel case, because then they are
> > a bit shorter.
> > If camel case is unwanted, for sure we need that change.
>
> Camel case are unwanted.
>
> >
> > Please mind the allignment. For enhanced readability of structs, I always
> > try to start the type
> > in the same column, the variable name in the same column and - if nneded -
> > the comments in the
> > same column - so you see all members of the struct optically in a kind of
> > table.
>
> Rishabh is going to have to redo the patchset anyway so don't feel bad
> about asking for changes. Put these review comments next to the change
> you are complaining about.
>
> No top posting.
>
> regards,
> dan carpenter
>
>
>