Source-Changes-D archive

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index][Old Index]

Re: CVS commit: src



Lourival Vieira Neto wrote:
> Yes, it isn't. But, please note, I didn't change that now. I just
> merged it in one single file. Though I think we need implement integer
> exponentiation, I think that is not a priority. IMO, it is a TODO.

Well, I assume that any raised issues should be resolved before moving
broken stuff around. See the link to my review below.

If you wanted to keep this item in TODO list, you'd better picked
lua_error rather than lua_mul ;-)


> > 2. If intmax_t is a greater type than int64_t, the below doesn't
> > handle overflow:
> > #define lua_str2number(s,p)    ((int64_t) strtoimax((s), (p), 10))
> 
> I'm considering two approaches:
> 
> 1) creating an auxiliary function based on strtoimax():
...
> 2) creating an auxiliary function based on strtol template

I think you created some problems for yourself by using int64_t. I was
going to tell you that Lua 5.3 uses long long for 64bit integers but I
saw your question on lua-l about it.

I didn't mind using long long in the kernel when we discussed that topic
but some other people convinced you to use explicit width type ;-)

> > 3. Item 1 was in my earlier review of luaconf.h in sys/modules and I see
> > at least one minor item not covered by your new change. Can you please
> > go over my review and make sure all covered?
> 
> Sorry, I've missed that. Which review? Are you talking about the
> "changing lua_Number to int64_t" thread?

http://mail-index.netbsd.org/source-changes-d/2013/10/22/msg006172.html

Alex


Home | Main Index | Thread Index | Old Index