Make one more function inine and use a table instead of switch/case#60
Make one more function inine and use a table instead of switch/case#60masatake merged 2 commits intouniversal-ctags:masterfrom
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR refactors escape sequence handling to improve performance by replacing switch/case statements with a lookup table and marking hot path functions as inline.
- Replaces switch/case with a
altcharslookup table for escape sequence handling - Adds
READTAGS_INLINEto two hot path functions:readTagCharacterandunescapeInPlace - Updates performance benchmarks showing ~40% improvement in execution time
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| readtags.c | Introduces lookup table for escape sequences, adds inline keywords to hot path functions |
| NEWS.md | Documents performance improvements with updated benchmark results |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| case 'x': | ||
| if (isxdigit (p[1]) && isxdigit (p[2])) | ||
| default: | ||
| a = altchars[*p]; |
There was a problem hiding this comment.
Potential out-of-bounds array access. The pointer *p is a signed char that's cast to unsigned char for array indexing, but there's no validation that *p is within the valid array bounds before accessing altchars[*p]. If *p contains a value >= 255, this will access memory beyond the array (especially problematic given the array size issue in Comment 1).
| a = altchars[*p]; | |
| a = (*p < 0xff) ? altchars[*p] : 0; |
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| unsigned char a; | ||
| switch (*p) | ||
| { | ||
| case 't': c = '\t'; p++; break; | ||
| case 'r': c = '\r'; p++; break; | ||
| case 'n': c = '\n'; p++; break; | ||
| case '\\': c = '\\'; p++; break; | ||
| /* Universal-CTags extensions */ | ||
| case 'a': c = '\a'; p++; break; | ||
| case 'b': c = '\b'; p++; break; | ||
| case 'v': c = '\v'; p++; break; | ||
| case 'f': c = '\f'; p++; break; | ||
| case 'x': | ||
| if (isxdigit (p[1]) && isxdigit (p[2])) | ||
| default: | ||
| a = altchars[*p]; | ||
| if (a) { | ||
| c = a; | ||
| p++; | ||
| } | ||
| break; |
There was a problem hiding this comment.
The switch statement structure is incorrect. The default case is now positioned before the case 'x': statement (line 195), which means the hexadecimal escape sequence handling will never be reached. The default case should be moved after all other case statements.
Signed-off-by: Masatake YAMATO <yamato@redhat.com>
Signed-off-by: Masatake YAMATO <yamato@redhat.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #60 +/- ##
==========================================
+ Coverage 77.94% 78.15% +0.20%
==========================================
Files 16 16
Lines 1705 1703 -2
Branches 153 154 +1
==========================================
+ Hits 1329 1331 +2
+ Misses 376 372 -4 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
The coverage is much lower than I expected. |
No description provided.