This is the mail archive of the libc-alpha@sourceware.org mailing list for the glibc project.
Index Nav: | [Date Index] [Subject Index] [Author Index] [Thread Index] | |
---|---|---|
Message Nav: | [Date Prev] [Date Next] | [Thread Prev] [Thread Next] |
Other format: | [Raw text] |
On Mon, May 26, 2014 at 07:56:38AM -0400, Carlos O'Donell wrote: > I went back and reviewed cyclomatic complexity, which applies here > as a correlative indicator of maintenance cost. > > The original function already by my rough count has a McCabe cyclomatic > complexity of 15, and any function >10 is complex to understand and > difficult for testing to go through all the if/else and loops conditions. > > Merging that into the existing function results in a function that is > even harder to understand and test. > > Cyclomatic complexity is the only metric I can bring to bear here in > order to provide some objectivity to our "feelings" of what's readable > or unreadable. > > The simpler the function the less time it takes to review, > so while you might argue "it's simple enough", that equates to some fixed > amount of time to read the code. If the function was *simpler* the > review and reading would be *faster* allowing anyone to go through the > code quickly. OK, I just have a higher threshold for cyclomatic complexity ;) But on a serious note, I don't disagree that the code is complicated, I only claim that the function breakout looks ugly. > > 3. Making a separate non-inline function would require passing of a > > lot of context to the function for just one call - you would end up > > with a function call with 10 arguments. That IMO is much more > > unwieldy to maintain than the simple inline bit. > > Why does this make it unwieldly to maintain? Because (as you guessed) it makes for a complicated internal API. > It makes for a complex internal API, I agree, but if you carefully > use `const' it also draws a pretty clean line between what you can > and can't change and makes easily visible how to refactor both functions > to make things simpler. There's not a lot to mark as const. In fact, everything is modified in the function AFAICT, so you'll essentially end up passing references to all counters into the function. > > That said, the best option for what I am planning to do with this > > function (add mmap stats and add an option to produce JSON) would be > > to collate all the statistics into a struct and pass to struct to a > > printer function when we're done collecting data. However, that is > > even more code rework, so it might be suitable for a separate pass. > > In which case I still advocate a distinct function with `const' arguments > where appropriate instead of inlining. Since anything else is a waste of > time because we want JSON stats and we're reworking the code? OK, lets just get the function out first. I don't think it's important enough to argue over right now since we're going to rework the code in later passes anyway. Ondrej, could you please do a second version of the patch with the function just popped out and the global counters passed as references? Thanks, Siddhesh
Attachment:
pgpTkxiC707IV.pgp
Description: PGP signature
Index Nav: | [Date Index] [Subject Index] [Author Index] [Thread Index] | |
---|---|---|
Message Nav: | [Date Prev] [Date Next] | [Thread Prev] [Thread Next] |