D29808: KSysGuard Network Plugin: Don't use std::regex to parse the network files.

[Jiří Paleček]

jpalecek added a comment.


  In D29808#673157 <https://phabricator.kde.org/D29808#673157>, @sandsmark wrote:
  
  > > It's all C code whereas the rest of the helper is C++. It also relies very heavily on magic numbers now.
  >
  >
  >
  > > I think a much simpler implementation would be to split each line on " ", select the fields we want and clean them up.
  >
  > I assume this is for performance reasons, but a tiny microbenchmark showing that it is actually faster would be nice.
  
  
  Hi! Just out of curiosity (I'm not the OP), I created a microbenchmark here: https://github.com/jpalecek/ksysguard-network-microbenchmark. As a fun project, I added a Boost.Sphinx implementation, which was remarkably easy. Not the most readable code ever, beware. Some bugs were found while doing this (in both old and new implementation), see the code.

INLINE COMMENTS

> sandsmark wrote in ConnectionMapping.cpp:167
> instead of 87 + 1, maybe have a `constexpr int lineLength = strlen("0: 00000000000000000000000001000000:0277 00000000000000000000000000000000:0000 0A 00000000:00000000 00:00000000 00000000     0        0 16741");` above with the comment?
> or:
> 
>   constexpr const char *exampleLine = "0: 00000000000000000000000001000000:0277 00000000000000000000000000000000:0000 0A 00000000:00000000 00:00000000 00000000     0        0 16741";
>       constexpr int lineLength = strlen(exampleLine);
> 
> both more understandable and less prone to accidentally putting in the wrong number.
> 
> same with all other magic string offset numbers here.

Actually I don't agree with that view. I don't think `lineLength = strlen("0: 00000000000000000000000...`)` is readable and less error prone than `87`, only longer by ~100 characters and a nightmare to check. With numbers, I can check with an editor that shows column numbers and simple arithmetic, but comparing such long strings made of spaces is hard.

If you want more robustness, I would suggest using the fact that the number columns are aligned with the header and using something like `inode_col = header.find("inode")`. Or counting the fields. That would also take care of the possibility that the columns may not be same width across architectures (?)

REPOSITORY
  R106 KSysguard

REVISION DETAIL
  https://phabricator.kde.org/D29808

To: dkorth, ahiemstra
Cc: sandsmark, ahiemstra, jpalecek, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, zachus, fbampaloukas, ragreen, ZrenBot, ngraham, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/plasma-devel/attachments/20200524/53c289f7/attachment-0001.htm>