Re: [ros-dev] msvc build update

[Jose Catena]

Hi,

Don't feel offended, the port to c is a very good thing and indeed better in
the most part than the previous asm implementation. I value your work very
much.
But there are some aspects badly implemented:
1) It is not possible to write a good stub in an inline function nor in a
regular macro due to c preprocessor limitations. Your implementation trust
the optimizer to remove constant conditionals, but it does not work as well
as you may think. Have you seen the generated code? It has a lot of
unnecessary branching. I have implemented it as an x-macro, where I can use
the preprocessor to select the options to be generated and it does not
generate any run time conditional branching.
2) You have added many inline assembly parts in several files that should be
kept as portable as possible. These things are better put together in
architecture dependent files/directories. Furthermore, most are unnecessary,
my single trap stub in a single x-macro replaces most of your inlines. 
3) Since the pointer to the trap frame is passed to handlers, why do you
pass as extra parameters information that is already there? Access to
members of the KTRAP_FRAME is just as fast as access to local variables or
parameters, but saves on additional copies and allow more efficient register
usage. Furthermore using regparam(3) for functions called from inline
assembly, when actually only one param was needed, was really a very bad
idea. And you pass the same parameters to child handlers in cascade, again
and again. This is very unefficient. Today I simplified the syscall and
fastsyscall handlers removing a lot unnecessary parameter copying and
cascading, saving a lot of cycles and memory, while making it much easier to
read and understand.
4) Why did you resort to code patching to encode the PKINTERRUPT parameter,
instead of generating a static variable named together with the stub, for
example? Wasn't it hackish? I hope you understand that you won't save even a
cycle that way, so why to use such an unportable and complex method?
 
The worst thing is the indiscriminate and spread use of inline assembly and
compiler specific features. Please think in placing such things in platform
specific includes and try to make them as general and reusable as possible,
instead of making the code base very difficult to port and maintain.

Please take this as constructive criticism and not as any personal offense.
We all can always learn better if we can accept criticism.
 
Jose Catena
DIGIWAVES S.L.