Re: [PATCH 08/11] trace-cmd: Making start,extract,stream,profile separate funcs

[Vladislav Valtchev]

On Tue, 2017-11-28 at 12:14 -0500, Steven Rostedt wrote:	
> As everything is doing both init_common_record_context() and
> parse_record_options() why not just move the
> init_common_record_context() into parse_record_options(). If you need
> to do something special (like set events = 1 for profile) have that
> done in parse_record_options() if the cmd is CMD_record. Yes, you need
> to pass the CMD_* to parse_record_options() in this case.
> 

Actually, I spent some effort trying to avoid exactly that:
"if" statments in "overly generic code". In my view, the point
of having several trace_* functions is to be able to write specific code
for them, without conditional branches.

I understand that, clearly, there is a trade-off because the extreme of that is to
have a different copy of a function like parse_record_options() for each command.
On the other hand, when you have an IF (cmd is PROFILE) at the beginning and an
if (cmd is PROFILE) at the end, it looks to me a good thing to move that code in the 
specific trace_profile() function. It's all about finding the right balance:
the previous extreme (before the refactoring) was to have everything in trace_record()
with much more if statements and that looked more complicated to follow, at least for me.

Clearly, I fully understand that the location of that "right balance" is pretty subjective.

Do you have a strong opinion on that?


Vlad