cambridgehackers / connectal

Connectal is a framework for software-driven hardware development.
MIT License
161 stars 46 forks source link

Parser for includes. #132

Open threonorm opened 7 years ago

threonorm commented 7 years ago

Depending on how we comment an include :

//`include foo
// `include bar

the script for dependency tracking consider that foo is a dependency, but not bar. When I expect the two to not be dependencies (as they are commented).

I guess connectal used the first form of comment as a feature to signal weird dependies invisible to bluespec but visible to connectal?

The code involved is in bsvdependencies.py:

 m = re.match('//`include "([^\"]+)"', line)
 m1 = re.match('//`include(.*)', line)
 if m:
                iname = m.group(1)
                if iname in abspaths:
                    iname = abspaths[iname]
                else:
                    iname = 'obj/%s' % iname
                includes.append(iname)

I am not sure why this special case. I don't have the full picture. If connectal really need to do this kind of commented include that should not be seen by BSV, perhaps connectal could use a special form of include like `includeConnectal, that would be in the comment section?

hanw commented 7 years ago

Hi threonorm, Jamey is on vacation with limited email access, so your question may not be answered in time. It sounds like the error is due to some hacky-ness in bsvdependencies.py , would you be able suggest a fix for this? I think you are correct that none of these two cases should generate a dependency.

threonorm commented 7 years ago

Hi hanw, thanks for your answer!

I have a "fix" where I just consider every commented dependency to be not a dependency, this work for my use of the bsvdependencies functions, but I suspect that Jamey uses it in a different way somewhere else where he actually wants to keep this feature. (The way the code is written make me believe that this is really a feature)

For this reason, I don't want to pull request yet, waiting for his opinion is definitely a good idea :).

jameyhicks commented 7 years ago

I guess it would be less Jaclyn if bsvpreprocess returned a list of included files as well as the expanded source code. Do you have time to make that change? Then it could ignore all commented out includes.

jameyhicks commented 7 years ago

autocorrect strikes again :)

jameyhicks commented 7 years ago

In case you did not see the comments I made in one of the chat channels:

I think this is an artifact of the interaction between bsvpreprocess and bsvdepend. It's a bit too hackish, now, so probably it would be better if bsvpreprocess returned a list of the included files along with the preprocessed text and then we do not have to treat commented out includes specially.

Either that, or we need a much more unique tag for the commented includes. How about a commented out attribute? // ( included_file="filename.bsvi" )

It's too bad the BSV compiler does not support the equivalent of C/C++

line directive, which is used for just this purpose.

If you have a chance to make a pull request along either of these lines I would greatly appreciate it.

Happy holidays!

On Fri, Dec 2, 2016 at 3:03 PM threonorm notifications@github.com wrote:

Hi hanw, thanks for your answer!

I have a "fix" where I just consider every commented dependency to be not a dependency, this work for my use of the bsvdependencies functions, but I suspect that Jamey uses it in a different way somewhere else where he actually wants to keep this feature. (The way the code is written make me believe that this is really a feature)

For this reason, I don't want to pull request yet, waiting for his opinion is definitely a good idea :).

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/cambridgehackers/connectal/issues/132#issuecomment-264549277, or mute the thread https://github.com/notifications/unsubscribe-auth/ACU3s-ktGy7VqAi3VAL82uMkgZU-4Kluks5rEHmjgaJpZM4LC4WU .

threonorm commented 7 years ago

Hello Jamey,

I am not sure I understand the problem properly : I don't understand what is this special kind of include for and where/what it used for. (What are those include that are commented, but still considered as include?)

Though if abstractly speaking you just need a special kind of include, only seen by connectal build system, and added by connectal automatically somehow, I think I agree with your second suggestion, I can modify bsvdependencies to special pick something like //CONNECTALINCLUDE "nameoffile"

I can definitely make the one line change in bsvdependencies, but I would break everything by doing so, because I don't know where I should change things as well. (I guess some script add those special include for connectal somewhere : I think those special include are not hand written, are they ? And if they are, it means that I will break every project that is using them.).

Thomas

Excerpts from Jamey Hicks's message of 2016-12-30 08:10:02 -0500:

In case you did not see the comments I made in one of the chat channels:

I think this is an artifact of the interaction between bsvpreprocess and bsvdepend. It's a bit too hackish, now, so probably it would be better if bsvpreprocess returned a list of the included files along with the preprocessed text and then we do not have to treat commented out includes specially.

Either that, or we need a much more unique tag for the commented includes. How about a commented out attribute? // ( included_file="filename.bsvi" )

It's too bad the BSV compiler does not support the equivalent of C/C++

line directive, which is used for just this purpose.

If you have a chance to make a pull request along either of these lines I would greatly appreciate it.

Happy holidays!

On Fri, Dec 2, 2016 at 3:03 PM threonorm notifications@github.com wrote:

Hi hanw, thanks for your answer!

I have a "fix" where I just consider every commented dependency to be not a dependency, this work for my use of the bsvdependencies functions, but I suspect that Jamey uses it in a different way somewhere else where he actually wants to keep this feature. (The way the code is written make me believe that this is really a feature)

For this reason, I don't want to pull request yet, waiting for his opinion is definitely a good idea :).

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/cambridgehackers/connectal/issues/132#issuecomment-264549277, or mute the thread https://github.com/notifications/unsubscribe-auth/ACU3s-ktGy7VqAi3VAL82uMkgZU-4Kluks5rEHmjgaJpZM4LC4WU .