Opened 20 years ago
Closed 20 years ago
#701 closed enhancement (fixed)
Improve readibility and performance of msCopyMap
Reported by: | Owned by: | ||
---|---|---|---|
Priority: | normal | Milestone: | |
Component: | MapServer C Library | Version: | 4.3 |
Severity: | minor | Keywords: | |
Cc: | mturk@… |
Description
Move to use of macros rather than copyProperty function as suggested by Mladen Turk.
Attachments (2)
Change History (13)
by , 20 years ago
Attachment: | mapcopy.c.txt added |
---|
comment:1 by , 20 years ago
Cc: | added |
---|
MT, I am experiencing a problem with your MS_COPYSTELEM macro. Every time it is used, I get a warning: mapcopy.c:160:23: warning: pasting "->" and "type" does not give a valid preprocessing token and so on. I'm using gcc 3.2.2. You mentioned that your work is .NET oriented, so you must be using MSVC? We should find something that will work across platforms even if it's a bit less concise. Also, re your move of msCopySymbol from mapsymbol.c -> mapcopy.c ... that is where it started, but we are trying organize the code better and put all the symbol stuff together in one place. Since the map copying is fragile, it seems like keeping it nearer to the other object functions will remind developers to keep it updated if they make changes to the symbolObj structure.
comment:2 by , 20 years ago
The promlem is with the 'token-pasting' operator (##) in macro MS_COPYSTELEM. VC7 allows to be the first token in join, but seems that gcc don't. The solution is to replace all ocureneces of MS_COPYSTELEM(xxxx) with dst->xxxx = src->xxxx; I'll make a script and replace them all. Regarding msCopySymbol, that's fine, but in that case the (working) macros will have to go in map.h.
comment:3 by , 20 years ago
The macro that works for me is MS_COPYSTELEM(x) dst->/**/x = src->/**/x That's the gcc traditional form. The ISO macro you wrote *should* work. Can you think of a good way to detect which one should be used? I think that such a macro is useful. How about putting the macros in a new mapcopy.h instead of map.h?
comment:4 by , 20 years ago
Actually, what I meant to write is that the ISO macro *should not* work, even for MSVC, because it is supposed to join two valid tokens and neither src-> or dst-> are valid tokens.
comment:5 by , 20 years ago
Sure, the #define MS_COPYSTELEM(name) (dst)->/**/name = (src)->/**/name works both on gcc (GCC) 3.2.3 (mingw special 20030504-1) and MSVC. Seems that I've run 'sed' prior seeing your comments. The macro is quite good cause it resolves typo errors.
comment:7 by , 20 years ago
Status: | new → assigned |
---|
OK, I have added the macros to mapcopy.h and am now using them in all the msCopy* functions. There are a couple of odd cases where the MS_COPYSTRING by itself was not robust: 1) the elements of the args member of a projectionObj, 2) the symbolname member of a symbolObj. You can see how I worked around in mapsymbol.c:msCopySymbol() and mapcopy.c:msCopyProjection(). The testcopy program runs through valgrind with no errors and all the Python unit tests, including new cloning unit tests, pass. Looking forward to your feedback, MT. And thanks for the input, I think that resolving this bug (and 640) makes the software a lot better.
comment:9 by , 20 years ago
Just saw your comments on few problems with macros. Perhaps the problem might be resolved by prefixing macro operators with underscore. It will in that case resolve common naming conflicts, cause 'name' is quite common variable name, as well as dst and src. Also the MS_COPYSTRING macro has a bug for array copying, cause the 'if (src)' shoude be 'if ((src))'. Sorry, but I've miss that. Here is the slightly better version: #define MS_COPYSTELEM(_name) (dst)->/**/_name = (src)->/**/_name #define MS_MACROBEGIN do { #define MS_MACROEND } while (0) #define MS_COPYRECT(_dst, _src) \ MS_MACROBEGIN \ (_dst)->minx = (_src)->minx; \ (_dst)->miny = (_src)->miny; \ (_dst)->maxx = (_src)->maxx; \ (_dst)->maxy = (_src)->maxy; \ MS_MACROEND #define MS_COPYPOINT(_dst, _src) \ MS_MACROBEGIN \ (_dst)->x = (_src)->x; \ (_dst)->y = (_src)->y; \ (_dst)->m = (_src)->m; \ MS_MACROEND #define MS_COPYCOLOR(_dst, _src) \ MS_MACROBEGIN \ (_dst)->pen = (_src)->pen; \ (_dst)->red = (_src)->red; \ (_dst)->green = (_src)->green; \ (_dst)->blue = (_src)->blue; \ MS_MACROEND #define MS_COPYSTRING(_dst, _src) \ MS_MACROBEGIN \ if ((_dst) != NULL) \ msFree((_dst)); \ if ((_src)) \ (_dst) = strdup((_src)); \ else \ (_dst) = NULL; \ MS_MACROEND
comment:11 by , 20 years ago
Resolution: | → fixed |
---|---|
Status: | assigned → closed |
Closing this bug. Any problems with msCopyMap will be a new issue.
Note:
See TracTickets
for help on using tickets.
MT's new mapcopy.c file