[Github-comments] Re: [geany/geany] Fix crash by protecting tm_ctags_*() functions against TM_PARSER_NONE (PR #3865)
Merged #3865 into master. -- Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/3865#event-12947445320 You are receiving this because you are subscribed to this thread. Message ID:
[Github-comments] Re: [geany/geany] Fix crash by protecting tm_ctags_*() functions against TM_PARSER_NONE (PR #3865)
@b4n approved this pull request. -- Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/3865#pullrequestreview-2079821073 You are receiving this because you are subscribed to this thread. Message ID:
[Github-comments] Re: [geany/geany] Fix crash by protecting tm_ctags_*() functions against TM_PARSER_NONE (PR #3865)
> LGTM, apart possibly the inline comments. I've made the changes, amended the previous commit with it and force-pushed the result. > Sad we have to do that in several functions, but makes sense. The crash itself would probably not require all the places but better to have it handled everywhere so we don't run into it in the future. -- Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/3865#issuecomment-2132404446 You are receiving this because you are subscribed to this thread. Message ID:
[Github-comments] Re: [geany/geany] Fix crash by protecting tm_ctags_*() functions against TM_PARSER_NONE (PR #3865)
@techee commented on this pull request. > + kindDefinition *def; + + if (lang == TM_PARSER_NONE) + return "unknown"; + + def = getLanguageKindForLetter(lang, kind); Yes, that's better - done at both places. -- Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/3865#discussion_r1615332108 You are receiving this because you are subscribed to this thread. Message ID:
[Github-comments] Re: [geany/geany] Fix crash by protecting tm_ctags_*() functions against TM_PARSER_NONE (PR #3865)
@techee pushed 1 commit. dcd46bbe62e32d3ed1f6f25b977cdfeecd95ee2d Protect tm_ctags_*() functions against TM_PARSER_NONE language parameter -- View it on GitHub: https://github.com/geany/geany/pull/3865/files/5bb09fbd2a0a6aa4d4fec8a187e3b4f171cdbe0e..dcd46bbe62e32d3ed1f6f25b977cdfeecd95ee2d You are receiving this because you are subscribed to this thread. Message ID:
[Github-comments] Re: [geany/geany] Fix crash by protecting tm_ctags_*() functions against TM_PARSER_NONE (PR #3865)
@b4n commented on this pull request. LGTM, apart possibly the inline comments. Sad we have to do that in several functions, but makes sense. > + kindDefinition *def; + + if (lang == TM_PARSER_NONE) + return "unknown"; + + def = getLanguageKindForLetter(lang, kind); Maybe something like this to avoid repeating the fallback value? ```suggestion kindDefinition *def = NULL; if (lang != TM_PARSER_NONE) def = getLanguageKindForLetter(lang, kind); ``` > + kindDefinition *def; + + if (lang == TM_PARSER_NONE) + return '-'; + + def = getLanguageKindForName(lang, name); Ditto here, maybe: ```suggestion kindDefinition *def = NULL; if (lang != TM_PARSER_NONE) def = getLanguageKindForName(lang, name); ``` -- Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/3865#pullrequestreview-2068531076 You are receiving this because you are subscribed to this thread. Message ID:
[Github-comments] Re: [geany/geany] Fix crash by protecting tm_ctags_*() functions against TM_PARSER_NONE (PR #3865)
> The file tm_ctags.c is our source that hides the ctags "API" behind some thin > layer and it is only this file that includes ctags headers so we don't > pollute the rest of our sources with symbols included from ctags. Too easy then. -- Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/3865#issuecomment-2094795525 You are receiving this because you are subscribed to this thread. Message ID:
[Github-comments] Re: [geany/geany] Fix crash by protecting tm_ctags_*() functions against TM_PARSER_NONE (PR #3865)
> So the question goes back to, since the proposed changes are to ctags will > upstream accept the changes so we don't have to keep a patch to ctags? See the first answer here: https://github.com/geany/geany/pull/3865#issuecomment-2094775899 :-). The file `tm_ctags.c` is our source that hides the ctags "API" behind some thin layer and it is only this file that includes ctags headers so we don't pollute the rest of our sources with symbols included from ctags. -- Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/3865#issuecomment-2094794719 You are receiving this because you are subscribed to this thread. Message ID:
[Github-comments] Re: [geany/geany] Fix crash by protecting tm_ctags_*() functions against TM_PARSER_NONE (PR #3865)
> The custom filetypes that don't use tag_parser also set the parser to > TM_PARSER_NONE but don't have any global tags file so the crash doesn't > happen for them. Ok, it didn't make sense that TM_PARSER_NONE worked for custom, but not built-in, but in fact that we are doing something that just luck says doesn't happen to be used for custom filetypes. Presumably if someone made a hand made global tags file for a custom filetype it would crash as well. So yeah it needs to be protected. So the question goes back to, since the proposed changes are to ctags will upstream accept the changes so we don't have to keep a patch to ctags? -- Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/3865#issuecomment-2094792207 You are receiving this because you are subscribed to this thread. Message ID:
[Github-comments] Re: [geany/geany] Fix crash by protecting tm_ctags_*() functions against TM_PARSER_NONE (PR #3865)
> Sorry, I wasn't clear, many of the custom filetypes have no parser and don't > crash. So what do they set the tag_parser to? The crash only happened because Geany was trying to load the global tags file for Python (`data/tags/std.py.tags`) and it was Python for which I set `tag_parser=`. The custom filetypes that don't use `tag_parser` also set the parser to `TM_PARSER_NONE` but don't have any global tags file so the crash doesn't happen for them. I could have also fixed this particular crash by checking the parser for `TM_PARSER_NONE` inside e.g. `tm_workspace_load_global_tags ()` but I think it's a good idea to protect all the ctags-interfacing functions against this value. > And can tag_parser= be set to the same value that successfully works for > custom filetypes. Well, I think we should be able to allow users disable some ctags parser if it e.g. causes crashes and `tag_parser=` is the way to do it currently. And setting the parser of say the Pascal filetype to the C parser isn't a good idea as the C parser will be constantly confused by Pascal sources. -- Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/3865#issuecomment-2094788766 You are receiving this because you are subscribed to this thread. Message ID:
[Github-comments] Re: [geany/geany] Fix crash by protecting tm_ctags_*() functions against TM_PARSER_NONE (PR #3865)
Sorry, I wasn't clear, many of the custom filetypes have no parser and don't crash. So what do they set the `tag_parser` to? And can `tag_parser=` be set to the same value that successfully works for custom filetypes. -- Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/3865#issuecomment-2094783568 You are receiving this because you are subscribed to this thread. Message ID:
[Github-comments] Re: [geany/geany] Fix crash by protecting tm_ctags_*() functions against TM_PARSER_NONE (PR #3865)
> For most languages the parser is hard-coded using the table here: ... > I think only a few non-builtin filetypes like JSON use the tag_parser option > to specify the parser by name. But if the name is invalid or not provided, we > shouldn't crash which this PR tries to do. And also the hard-coded parsers can be overridden by this option so by specifying e.g. `tag_parser=Python` for the C filetype, we'd start using the Python parser for it (doesn't make much sense in this case). So by specifying `tag_parser=` we override the C parser to "no parser" so TM is completely disabled for C and LSP doesn't interfere with it. -- Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/3865#issuecomment-2094777501 You are receiving this because you are subscribed to this thread. Message ID:
[Github-comments] Re: [geany/geany] Fix crash by protecting tm_ctags_*() functions against TM_PARSER_NONE (PR #3865)
> What does upstream say about the changes? The change is in our sources, not the upstream ones. > What is the setting set to if the tag_parser= is not present? For most languages the parser is hard-coded using the table here: https://github.com/geany/geany/blob/ef2255bced523a3ad75773bac3e77b02725f10fd/src/filetypes.c#L116 I think only a few non-builtin filetypes like JSON use the `tag_parser` option to specify the parser by name. But if the name is invalid or not provided, we shouldn't crash which this PR tries to do. > And can tag_parser= mean that? Mean what? -- Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/3865#issuecomment-2094775899 You are receiving this because you are subscribed to this thread. Message ID:
[Github-comments] Re: [geany/geany] Fix crash by protecting tm_ctags_*() functions against TM_PARSER_NONE (PR #3865)
What does upstream say about the changes? What is the setting set to if the `tag_parser=` is not present? And can `tag_parser=` mean that? -- Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/3865#issuecomment-2094556791 You are receiving this because you are subscribed to this thread. Message ID:
